Merge pull request #207 from blend-capital/backstop-fixes

backstop: minimize inflation attack exploit (#196)

backstop/src/backstop/deposit.rs

@@ -1,4 +1,6 @@
-use crate::{contract::require_nonnegative, emissions, storage, BackstopError};
+use crate::{
+    constants::MIN_INITIAL_SHARES, contract::require_nonnegative, emissions, storage, BackstopError,
+};
 use sep_41_token::TokenClient;
 use soroban_sdk::{panic_with_error, Address, Env};
 
@@ -20,6 +22,9 @@ pub fn execute_deposit(e: &Env, from: &Address, pool_address: &Address, amount:
     backstop_token_client.transfer(from, &e.current_contract_address(), &amount);
 
     let to_mint = pool_balance.convert_to_shares(amount);
+    if to_mint == 0 || (pool_balance.shares == 0 && to_mint < MIN_INITIAL_SHARES) {
+        panic_with_error!(e, &BackstopError::InvalidShareMintAmount);
+    }
     pool_balance.deposit(amount, to_mint);
     user_balance.add_shares(to_mint);
 
@@ -35,6 +40,7 @@ mod tests {
 
     use crate::{
         backstop::execute_donate,
+        constants::SCALAR_7,
         testutils::{create_backstop, create_backstop_token, create_mock_pool_factory},
     };
 
@@ -208,4 +214,65 @@ mod tests {
             execute_deposit(&e, &samwise, &pool_0_id, 100);
         });
     }
+
+    #[test]
+    #[should_panic(expected = "Error(Contract, #1005)")]
+    fn test_execute_deposit_zero_share_mint() {
+        let e = Env::default();
+        e.budget().reset_unlimited();
+        e.mock_all_auths_allowing_non_root_auth();
+
+        let backstop_address = create_backstop(&e);
+        let bombadil = Address::generate(&e);
+        let samwise = Address::generate(&e);
+        let frodo = Address::generate(&e);
+        let pool_0_id = Address::generate(&e);
+        let pool_1_id = Address::generate(&e);
+
+        let (_, backstop_token_client) = create_backstop_token(&e, &backstop_address, &bombadil);
+        backstop_token_client.mint(&samwise, &100_0000000);
+        backstop_token_client.mint(&frodo, &100_000_000_0000000);
+
+        let (_, mock_pool_factory_client) = create_mock_pool_factory(&e, &backstop_address);
+        mock_pool_factory_client.set_pool(&pool_0_id);
+        mock_pool_factory_client.set_pool(&pool_1_id);
+
+        // initialize pool 0 with funds + some profit
+        e.as_contract(&backstop_address, || {
+            execute_deposit(&e, &frodo, &pool_0_id, SCALAR_7);
+            execute_donate(&e, &frodo, &pool_0_id, 10_000_000 * SCALAR_7);
+        });
+
+        e.as_contract(&backstop_address, || {
+            execute_deposit(&e, &samwise, &pool_0_id, SCALAR_7);
+        });
+    }
+
+    #[test]
+    #[should_panic(expected = "Error(Contract, #1005)")]
+    fn test_execute_deposit_small_initial_mint() {
+        let e = Env::default();
+        e.budget().reset_unlimited();
+        e.mock_all_auths_allowing_non_root_auth();
+
+        let backstop_address = create_backstop(&e);
+        let bombadil = Address::generate(&e);
+        let samwise = Address::generate(&e);
+        let frodo = Address::generate(&e);
+        let pool_0_id = Address::generate(&e);
+        let pool_1_id = Address::generate(&e);
+
+        let (_, backstop_token_client) = create_backstop_token(&e, &backstop_address, &bombadil);
+        backstop_token_client.mint(&samwise, &100_0000000);
+        backstop_token_client.mint(&frodo, &100_0000000);
+
+        let (_, mock_pool_factory_client) = create_mock_pool_factory(&e, &backstop_address);
+        mock_pool_factory_client.set_pool(&pool_0_id);
+        mock_pool_factory_client.set_pool(&pool_1_id);
+
+        e.as_contract(&backstop_address, || {
+            execute_donate(&e, &frodo, &pool_0_id, SCALAR_7);
+            execute_deposit(&e, &samwise, &pool_0_id, SCALAR_7 / 10 - 1);
+        });
+    }
 }

backstop/src/backstop/pool.rs

@@ -57,8 +57,6 @@ pub fn require_is_from_pool_factory(e: &Env, address: &Address, balance: i128) {
     }
 }
 
-/// TODO: Duplicated from pool/pool/status.rs. Can this be moved to a common location?
-///
 /// Calculate the threshold for the pool's backstop balance
 ///
 /// Returns true if the pool's backstop balance is above the threshold
@@ -134,8 +132,6 @@ impl PoolBalance {
 
     /// Deposit tokens and shares into the pool
     ///
-    /// If this is the first time
-    ///
     /// ### Arguments
     /// * `tokens` - The amount of tokens to add
     /// * `shares` - The amount of shares to add

backstop/src/backstop/withdrawal.rs

@@ -1,6 +1,6 @@
-use crate::{contract::require_nonnegative, emissions, storage};
+use crate::{contract::require_nonnegative, emissions, storage, BackstopError};
 use sep_41_token::TokenClient;
-use soroban_sdk::{unwrap::UnwrapOptimized, Address, Env};
+use soroban_sdk::{panic_with_error, unwrap::UnwrapOptimized, Address, Env};
 
 use super::Q4W;
 
@@ -56,6 +56,9 @@ pub fn execute_withdraw(e: &Env, from: &Address, pool_address: &Address, amount:
     user_balance.dequeue_shares_for_withdrawal(e, amount, true);
 
     let to_return = pool_balance.convert_to_tokens(amount);
+    if to_return == 0 {
+        panic_with_error!(e, &BackstopError::InvalidTokenWithdrawAmount);
+    }
     pool_balance.withdraw(e, to_return, amount);
 
     storage::set_user_balance(e, pool_address, from, &user_balance);
@@ -75,7 +78,7 @@ mod tests {
     };
 
     use crate::{
-        backstop::{execute_deposit, execute_donate},
+        backstop::{execute_deposit, execute_donate, execute_draw},
         testutils::{
             assert_eq_vec_q4w, create_backstop, create_backstop_token, create_mock_pool_factory,
         },
@@ -363,6 +366,7 @@ mod tests {
             assert_eq!(backstop_token_client.balance(&samwise), tokens);
         });
     }
+
     #[test]
     #[should_panic(expected = "Error(Contract, #8)")]
     fn test_execute_withdrawal_negative_amount() {
@@ -413,4 +417,58 @@ mod tests {
             execute_withdraw(&e, &samwise, &pool_address, -42_0000000);
         });
     }
+
+    #[test]
+    #[should_panic(expected = "Error(Contract, #1006)")]
+    fn test_execute_withdrawal_zero_tokens() {
+        let e = Env::default();
+        e.mock_all_auths_allowing_non_root_auth();
+
+        let backstop_address = create_backstop(&e);
+        let pool_address = Address::generate(&e);
+        let bombadil = Address::generate(&e);
+        let samwise = Address::generate(&e);
+        let frodo = Address::generate(&e);
+
+        let (_, backstop_token_client) = create_backstop_token(&e, &backstop_address, &bombadil);
+        backstop_token_client.mint(&samwise, &150_0000000);
+        backstop_token_client.mint(&frodo, &150_0000000);
+
+        let (_, mock_pool_factory_client) = create_mock_pool_factory(&e, &backstop_address);
+        mock_pool_factory_client.set_pool(&pool_address);
+
+        e.ledger().set(LedgerInfo {
+            protocol_version: 20,
+            sequence_number: 200,
+            timestamp: 10000,
+            network_id: Default::default(),
+            base_reserve: 10,
+            min_temp_entry_ttl: 10,
+            min_persistent_entry_ttl: 10,
+            max_entry_ttl: 2000000,
+        });
+
+        // setup pool with queue for withdrawal and allow the backstop to incur a profit
+        e.as_contract(&backstop_address, || {
+            execute_deposit(&e, &frodo, &pool_address, 1_0000001);
+            execute_deposit(&e, &samwise, &pool_address, 1_0000000);
+            execute_queue_withdrawal(&e, &samwise, &pool_address, 1_0000000);
+            execute_draw(&e, &pool_address, 1_9999999, &frodo);
+        });
+
+        e.ledger().set(LedgerInfo {
+            protocol_version: 20,
+            sequence_number: 200,
+            timestamp: 10000 + 21 * 24 * 60 * 60 + 1,
+            network_id: Default::default(),
+            base_reserve: 10,
+            min_temp_entry_ttl: 10,
+            min_persistent_entry_ttl: 10,
+            max_entry_ttl: 2000000,
+        });
+
+        e.as_contract(&backstop_address, || {
+            execute_withdraw(&e, &samwise, &pool_address, 1_0000000);
+        });
+    }
 }

backstop/src/constants.rs

@@ -1,5 +1,8 @@
 /// Fixed-point scalar for 7 decimal numbers
 pub const SCALAR_7: i128 = 1_0000000;
 
+/// The minimum number of shares that can be created
+pub const MIN_INITIAL_SHARES: i128 = SCALAR_7 / 10;
+
 // The approximate deployment date of the backstop module TODO: pick one
 pub const BACKSTOP_EPOCH: u64 = 1441065600;

backstop/src/errors.rs

@@ -22,4 +22,6 @@ pub enum BackstopError {
     InvalidRewardZoneEntry = 1002,
     InsufficientFunds = 1003,
     NotPool = 1004,
+    InvalidShareMintAmount = 1005,
+    InvalidTokenWithdrawAmount = 1006,
 }

test-suites/tests/test_backstop_inflation_attack.rs

@@ -0,0 +1,107 @@
+#![cfg(test)]
+
+use soroban_sdk::{testutils::Address as _, vec, Address, Error, Symbol};
+use test_suites::{
+    pool::default_reserve_metadata,
+    test_fixture::{TestFixture, TokenIndex, SCALAR_7},
+};
+
+// This test showcases an inflation attack during a backstops initialization stage. This is a 
+// high risk inflation attack for the attacker, and requires significant capital to execute. 
+// If the donation does not happen before the victim deposits, the attacker will lose the majority
+// of the capital they donated to the backstop.
+//
+// However, since the attack is feasible, this test exists to ensure it's effects are minimized
+// and common pitfalls are avoided.
+#[test]
+fn test_backstop_inflation_attack() {
+    let mut fixture = TestFixture::create(false);
+
+    let whale = Address::generate(&fixture.env);
+    let sauron = Address::generate(&fixture.env);
+    let pippen = Address::generate(&fixture.env);
+
+    // create pool with 1 new reserve
+    fixture.create_pool(Symbol::new(&fixture.env, "Teapot"), 0, 6);
+
+    let xlm_config = default_reserve_metadata();
+    fixture.create_pool_reserve(0, TokenIndex::XLM, &xlm_config);
+    let pool_address = fixture.pools[0].pool.address.clone();
+
+    // setup backstop and update pool status
+    fixture.tokens[TokenIndex::BLND].mint(&whale, &(5_001_000 * SCALAR_7));
+    fixture.tokens[TokenIndex::USDC].mint(&whale, &(121_000 * SCALAR_7));
+    fixture.lp.join_pool(
+        &(400_000 * SCALAR_7),
+        &vec![&fixture.env, 5_001_000 * SCALAR_7, 121_000 * SCALAR_7],
+        &whale,
+    );
+
+    // execute inflation attack against pippen
+    let starting_balance = 200_000 * SCALAR_7;
+    fixture.lp.transfer(&whale, &sauron, &starting_balance);
+    fixture.lp.transfer(&whale, &pippen, &starting_balance);
+
+    // 1. Attacker deposits a small amount as the initial depositor
+
+    // contracts stop very small initial deposits
+    let bad_init_deposit =
+        fixture
+            .backstop
+            .try_deposit(&sauron, &pool_address, &(SCALAR_7 / 10 - 1));
+    assert_eq!(
+        bad_init_deposit.err(),
+        Some(Ok(Error::from_contract_error(1005)))
+    );
+
+    let sauron_deposit_amount = SCALAR_7 / 10;
+    let sauron_shares = fixture
+        .backstop
+        .deposit(&sauron, &pool_address, &sauron_deposit_amount);
+
+    // 2. Attacker donates a large amount to the backstop before the victim can perform a deposit
+    let inflation_amount = 100_000 * SCALAR_7;
+    fixture
+        .backstop
+        .donate(&sauron, &pool_address, &inflation_amount);
+
+    // contracts stop any zero share deposits
+    let mut deposit_amount = 1000;
+    let bad_deposit_result = fixture
+        .backstop
+        .try_deposit(&pippen, &pool_address, &deposit_amount);
+    assert_eq!(
+        bad_deposit_result.err(),
+        Some(Ok(Error::from_contract_error(1005)))
+    );
+
+    // user can still be in a situation where they get adversely affected by the inflation attacks
+    // but to a small extent
+    deposit_amount = SCALAR_7;
+    let pippen_shares = fixture
+        .backstop
+        .deposit(&pippen, &pool_address, &deposit_amount);
+    assert_eq!(pippen_shares, 9); // actual is 9.99...
+
+    // 3. Attacker and victim withdraw funds
+    fixture
+        .backstop
+        .queue_withdrawal(&sauron, &pool_address, &sauron_shares);
+    fixture
+        .backstop
+        .queue_withdrawal(&pippen, &pool_address, &pippen_shares);
+
+    // wait enough time so all shares can be withdrawn
+    fixture.jump(21 * 24 * 60 * 60 + 1);
+
+    fixture
+        .backstop
+        .withdraw(&sauron, &pool_address, &sauron_shares);
+    fixture
+        .backstop
+        .withdraw(&pippen, &pool_address, &pippen_shares);
+
+    // pippen loses less than 10% of initial deposit due to rounding
+    assert!(fixture.lp.balance(&sauron) < starting_balance + SCALAR_7 / 10);
+    assert!(fixture.lp.balance(&pippen) > starting_balance - SCALAR_7 / 10);
+}

test-suites/tests/test_pool_inflation_attack.rs

@@ -58,7 +58,7 @@ fn test_pool_inflation_attack() {
     fixture.jump_with_sequence(5);
 
     // 2. Attacker frontruns victim's deposit by depositing a large amount of underlying
-    //    to force the victim's minted shares to be zero.
+    //    to try and force an error in minting B tokens
     let inflation_amount = 100 * SCALAR_7;
     fixture.tokens[TokenIndex::XLM].transfer(
         &sauron,