Merge pull request #237 from Phoenix-Protocol-Group/217-v-phx-vul-017…

…-user-can-accidentally-swap-the-wrong-asset Pool and Pool stable: adds check if the swapped token is actualy one of the two tokens in the liquidity pool.
Phoenix-Protocol-Group · Feb 22, 2024 · b9b2a7e · b9b2a7e
2 parents 235c17e + 297aef6
commit b9b2a7e
Show file tree

Hide file tree

Showing 7 changed files with 162 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,12 +40,14 @@ and this project adheres to
 ## Bug fixes
 
 - Pool stable: Fixes an error in the compute_swap function, where commission isn't deducted ([233])
+- Pool and Pool stable: extend check if the swapped token is within pool ([#237])
 - Pool and Pool stable: adds missing validation of max_spread in do_swap ([#239])
 - Pool: Adds a validation for when shares can be zero during withdrawal ([#245])
 - Pool: adds new check that verifies the input params for get_deposit_amounts ([#246])
 - Stake: Adds access control to create distribution flow ([#249])
 
 [#233]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/233
+[#237]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/237
 [#239]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/239
 [#245]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/245
 [#246]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/246

diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs
@@ -594,8 +594,11 @@ impl LiquidityPoolTrait for LiquidityPool {
         let pool_balance_b = utils::get_pool_balance_b(&env);
         let (pool_balance_offer, pool_balance_ask) = if offer_asset == config.token_a {
             (pool_balance_a, pool_balance_b)
-        } else {
+        } else if offer_asset == config.token_b {
             (pool_balance_b, pool_balance_a)
+        } else {
+            log!(&env, "Token offered to swap not found in Pool");
+            panic_with_error!(env, ContractError::AssetNotInPool);
         };
 
         let compute_swap: ComputeSwap = compute_swap(
@@ -629,8 +632,11 @@ impl LiquidityPoolTrait for LiquidityPool {
         let pool_balance_b = utils::get_pool_balance_b(&env);
         let (pool_balance_offer, pool_balance_ask) = if ask_asset == config.token_b {
             (pool_balance_a, pool_balance_b)
-        } else {
+        } else if ask_asset == config.token_a {
             (pool_balance_b, pool_balance_a)
+        } else {
+            log!(&env, "Token offered to swap not found in Pool");
+            panic_with_error!(env, ContractError::AssetNotInPool);
         };
 
         let (offer_amount, spread_amount, commission_amount) = compute_offer_amount(
@@ -680,8 +686,10 @@ fn do_swap(
 
     let (pool_balance_sell, pool_balance_buy) = if offer_asset == config.token_a {
         (pool_balance_a, pool_balance_b)
-    } else {
+    } else if offer_asset == config.token_b {
         (pool_balance_b, pool_balance_a)
+    } else {
+        panic_with_error!(env, ContractError::AssetNotInPool);
     };
 
     // FIXM: Disable Referral struct
@@ -808,6 +816,11 @@ fn split_deposit_based_on_pool_ratio(
     deposit: i128,
     offer_asset: &Address,
 ) -> (i128, i128) {
+    // check if offer_asset is one of the two tokens in the pool
+    if offer_asset != &config.token_a && offer_asset != &config.token_b {
+        panic_with_error!(env, ContractError::AssetNotInPool);
+    }
+
     // Validate the inputs
     if a_pool <= 0 || b_pool <= 0 || deposit <= 0 {
         log!(

diff --git a/contracts/pool/src/error.rs b/contracts/pool/src/error.rs
@@ -22,4 +22,5 @@ pub enum ContractError {
     TotalSharesEqualZero = 13,
     DesiredAmountsBelowOrEqualZero = 14,
     MinAmountsBelowZero = 15,
+    AssetNotInPool = 16,
 }
diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs
@@ -811,3 +811,121 @@ fn test_v_phx_vul_021_should_panic_when_max_spread_invalid_range(max_spread: Opt
         &max_spread,
     );
 }
+
+#[test]
+#[should_panic(expected = "HostError: Error(Contract, #16)")]
+fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() {
+    let env = Env::default();
+    env.mock_all_auths();
+    env.budget().reset_unlimited();
+
+    let mut admin1 = Address::generate(&env);
+    let mut admin2 = Address::generate(&env);
+
+    let mut token1 = deploy_token_contract(&env, &admin1);
+    let mut token2 = deploy_token_contract(&env, &admin2);
+    let bad_token = deploy_token_contract(&env, &Address::generate(&env));
+
+    if token2.address < token1.address {
+        std::mem::swap(&mut token1, &mut token2);
+        std::mem::swap(&mut admin1, &mut admin2);
+    }
+    let user1 = Address::generate(&env);
+    let pool = deploy_liquidity_pool_contract(
+        &env,
+        None,
+        (&token1.address, &token2.address),
+        0i64,
+        None,
+        None,
+        None,
+        Address::generate(&env),
+        Address::generate(&env),
+    );
+    // Swap fails because we provide incorrect token as offer token.
+    pool.swap(
+        &user1,
+        // FIXM: Disable Referral struct
+        // &None::<Referral>,
+        &bad_token.address,
+        &1,
+        &None,
+        &Some(100),
+    );
+}
+
+#[test]
+#[should_panic(expected = "HostError: Error(Contract, #16)")]
+fn test_v_phx_vul_017_should_panic_when_simulating_swap_for_non_existing_token_in_pool() {
+    let env = Env::default();
+    env.mock_all_auths();
+    env.budget().reset_unlimited();
+
+    let mut admin1 = Address::generate(&env);
+    let mut admin2 = Address::generate(&env);
+
+    let mut token1 = deploy_token_contract(&env, &admin1);
+    let mut token2 = deploy_token_contract(&env, &admin2);
+    let bad_token = deploy_token_contract(&env, &Address::generate(&env));
+
+    if token2.address < token1.address {
+        std::mem::swap(&mut token1, &mut token2);
+        std::mem::swap(&mut admin1, &mut admin2);
+    }
+    let pool = deploy_liquidity_pool_contract(
+        &env,
+        None,
+        (&token1.address, &token2.address),
+        0i64,
+        None,
+        None,
+        None,
+        Address::generate(&env),
+        Address::generate(&env),
+    );
+    // Simulate swap fails because we provide incorrect token as offer token.
+    pool.simulate_swap(
+        // FIXM: Disable Referral struct
+        // &None::<Referral>,
+        &bad_token.address,
+        &1,
+    );
+}
+
+#[test]
+#[should_panic(expected = "HostError: Error(Contract, #16)")]
+fn test_v_phx_vul_017_should_panic_when_simulating_reverse_swap_for_non_existing_token_in_pool() {
+    let env = Env::default();
+    env.mock_all_auths();
+    env.budget().reset_unlimited();
+
+    let mut admin1 = Address::generate(&env);
+    let mut admin2 = Address::generate(&env);
+
+    let mut token1 = deploy_token_contract(&env, &admin1);
+    let mut token2 = deploy_token_contract(&env, &admin2);
+    let bad_token = deploy_token_contract(&env, &Address::generate(&env));
+
+    if token2.address < token1.address {
+        std::mem::swap(&mut token1, &mut token2);
+        std::mem::swap(&mut admin1, &mut admin2);
+    }
+    let pool = deploy_liquidity_pool_contract(
+        &env,
+        None,
+        (&token1.address, &token2.address),
+        0i64,
+        None,
+        None,
+        None,
+        Address::generate(&env),
+        Address::generate(&env),
+    );
+    // Simulate swap fails because we provide incorrect token as offer token.
+    pool.simulate_reverse_swap(
+        // FIXM: Disable Referral struct
+        // &None::<Referral>,
+        &bad_token.address,
+        &1,
+    );
+}
diff --git a/contracts/pool_stable/src/contract.rs b/contracts/pool_stable/src/contract.rs
@@ -1,8 +1,9 @@
 use phoenix::utils::LiquidityPoolInitInfo;
 use soroban_sdk::{
-    contract, contractimpl, contractmeta, log, Address, BytesN, Env, IntoVal, String,
+    contract, contractimpl, contractmeta, log, panic_with_error, Address, BytesN, Env, IntoVal,
 };
 
+use crate::error::ContractError;
 use crate::storage::utils::{is_initialized, set_initialized};
 use crate::storage::StableLiquidityPoolInfo;
 use crate::{
@@ -526,8 +527,10 @@ impl StableLiquidityPoolTrait for StableLiquidityPool {
         let pool_balance_b = utils::get_pool_balance_b(&env);
         let (pool_balance_offer, pool_balance_ask) = if offer_asset == config.token_a {
             (pool_balance_a, pool_balance_b)
-        } else {
+        } else if offer_asset == config.token_b {
             (pool_balance_b, pool_balance_a)
+        } else {
+            panic_with_error!(env, ContractError::AssetNotInPool);
         };
 
         let (ask_amount, spread_amount, commission_amount) = compute_swap(
@@ -557,10 +560,12 @@ impl StableLiquidityPoolTrait for StableLiquidityPool {
 
         let pool_balance_a = utils::get_pool_balance_a(&env);
         let pool_balance_b = utils::get_pool_balance_b(&env);
-        let (pool_balance_offer, pool_balance_ask) = if offer_asset == config.token_a {
+        let (pool_balance_offer, pool_balance_ask) = if offer_asset == config.token_b {
             (pool_balance_a, pool_balance_b)
-        } else {
+        } else if offer_asset == config.token_a {
             (pool_balance_b, pool_balance_a)
+        } else {
+            panic_with_error!(env, ContractError::AssetNotInPool);
         };
 
         let (offer_amount, spread_amount, commission_amount) = compute_offer_amount(
@@ -589,6 +594,10 @@ fn do_swap(
 ) -> i128 {
     let config = get_config(&env);
 
+    if offer_asset != config.token_a && offer_asset != config.token_b {
+        panic!("Trying to swap wrong asset. Aborting..")
+    }
+
     if let Some(max_spread) = max_spread {
         if !(0..=config.max_allowed_spread_bps).contains(&max_spread) {
             panic!("max spread is out of bounds")
@@ -603,8 +612,10 @@ fn do_swap(
 
     let (pool_balance_sell, pool_balance_buy) = if offer_asset == config.token_a {
         (pool_balance_a, pool_balance_b)
-    } else {
+    } else if offer_asset == config.token_b {
         (pool_balance_b, pool_balance_a)
+    } else {
+        panic_with_error!(env, ContractError::AssetNotInPool);
     };
 
     let (return_amount, spread_amount, commission_amount) = compute_swap(

diff --git a/contracts/pool_stable/src/error.rs b/contracts/pool_stable/src/error.rs
@@ -0,0 +1,8 @@
+use soroban_sdk::contracterror;
+
+#[contracterror]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum ContractError {
+    AssetNotInPool = 1,
+}
diff --git a/contracts/pool_stable/src/lib.rs b/contracts/pool_stable/src/lib.rs
@@ -1,5 +1,6 @@
 #![no_std]
 mod contract;
+mod error;
 mod math;
 mod storage;