From 348ad17b24855b824ab8768202f2a4d08545cf5a Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Thu, 8 Feb 2024 23:06:32 +0200 Subject: [PATCH 01/13] Pool and Pool stable: adds check if the swapped token is actualy one of the two tokens in the liquidity pool. --- contracts/pool/src/contract.rs | 16 ++++++++ contracts/pool/src/tests/swap.rs | 55 +++++++++++++++++++++++++++ contracts/pool_stable/src/contract.rs | 12 ++++++ 3 files changed, 83 insertions(+) diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index ce59d2a13..38772506e 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -583,6 +583,10 @@ impl LiquidityPoolTrait for LiquidityPool { fn simulate_swap(env: Env, offer_asset: Address, offer_amount: i128) -> SimulateSwapResponse { let config = get_config(&env); + if offer_asset != config.token_a && offer_asset != config.token_b { + panic!("Trying to swap wrong asset. Aborting..") + } + 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 { @@ -618,6 +622,10 @@ impl LiquidityPoolTrait for LiquidityPool { ) -> SimulateReverseSwapResponse { let config = get_config(&env); + if ask_asset != config.token_a && ask_asset != config.token_b { + panic!("Trying to swap wrong asset. Aborting..") + } + 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 ask_asset == config.token_b { @@ -652,6 +660,9 @@ fn do_swap( max_spread: Option, ) -> i128 { let config = get_config(&env); + if offer_asset != config.token_a && offer_asset != config.token_b { + panic!("Trying to swap wrong asset. Aborting..") + } // FIXM: Disable Referral struct // if let Some(referral) = &referral { // if referral.fee > config.max_referral_bps { @@ -801,6 +812,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!("Trying an operation with wrong asset. Aborting..") + } + // Validate the inputs if a_pool <= 0 || b_pool <= 0 || deposit <= 0 { log!( diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index 10cbe1034..f0bdcbe18 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -811,3 +811,58 @@ fn test_v_phx_vul_021_should_panic_when_max_spread_invalid_range(max_spread: Opt &max_spread, ); } + +#[test] +#[should_panic(expected = "Trying to swap wrong asset. Aborting..")] +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 swap_fees = 0i64; + let pool = deploy_liquidity_pool_contract( + &env, + None, + (&token1.address, &token2.address), + swap_fees, + None, + None, + None, + ); + + token1.mint(&user1, &1_001_000); + token2.mint(&user1, &1_001_000); + pool.provide_liquidity( + &user1, + &Some(1_000_000), + &Some(1_000_000), + &Some(1_000_000), + &Some(1_000_000), + &None, + ); + + // selling just one token with 1% max spread allowed + let spread = 100i64; // 1% maximum spread allowed + pool.swap( + &user1, + // FIXM: Disable Referral struct + // &None::, + &bad_token.address, + &1, + &None, + &Some(spread), + ); +} + diff --git a/contracts/pool_stable/src/contract.rs b/contracts/pool_stable/src/contract.rs index 0624aa980..7d7d059b2 100644 --- a/contracts/pool_stable/src/contract.rs +++ b/contracts/pool_stable/src/contract.rs @@ -514,6 +514,10 @@ impl StableLiquidityPoolTrait for StableLiquidityPool { fn simulate_swap(env: Env, offer_asset: Address, offer_amount: i128) -> SimulateSwapResponse { let config = get_config(&env); + if offer_asset != config.token_a && offer_asset != config.token_b { + panic!("Trying to swap wrong asset. Aborting..") + } + 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 { @@ -547,6 +551,10 @@ impl StableLiquidityPoolTrait for StableLiquidityPool { ) -> SimulateReverseSwapResponse { let config = get_config(&env); + if offer_asset != config.token_a && offer_asset != config.token_b { + panic!("Trying to swap wrong asset. Aborting..") + } + 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 { @@ -581,6 +589,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") From ecfa016bb28e801989f04f7dd604f65547709dae Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Thu, 8 Feb 2024 23:08:03 +0200 Subject: [PATCH 02/13] CHANGELOG.md: adds new entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a043febf7..3962ecaa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,8 +30,10 @@ and this project adheres to ## Added - Adds a new macro that validates the bps arguments value ([#199]) +- Pool and Pool stable: adds check if the swapped token is actualy one of the two tokens in the liquidity pool ([#237]) [#199]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/199 +[#237]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/237 ## Bug fixes From 7578344b096b12e022c8f34e02b7290b26d2e6da Mon Sep 17 00:00:00 2001 From: gangov Date: Fri, 9 Feb 2024 15:17:00 +0200 Subject: [PATCH 03/13] CHANGELOG.md: change for an entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3962ecaa1..a183322cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ and this project adheres to ## Added - Adds a new macro that validates the bps arguments value ([#199]) -- Pool and Pool stable: adds check if the swapped token is actualy one of the two tokens in the liquidity pool ([#237]) +- Pool and Pool stable: extend check if the swapped token is within pool ([#237]) [#199]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/199 [#237]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/237 From 5fb2e5d285dcd138f0426176ec80db7cd829fa53 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Fri, 9 Feb 2024 17:20:13 +0200 Subject: [PATCH 04/13] Pool and Pool stable: replaces panic! with panic_with_error! --- contracts/pool/src/contract.rs | 2 +- contracts/pool/src/tests/swap.rs | 1 - contracts/pool_stable/src/contract.rs | 7 +++++-- contracts/pool_stable/src/contracterror.rs | 8 ++++++++ contracts/pool_stable/src/lib.rs | 1 + 5 files changed, 15 insertions(+), 4 deletions(-) create mode 100644 contracts/pool_stable/src/contracterror.rs diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index 38772506e..2e18cf6aa 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -584,7 +584,7 @@ impl LiquidityPoolTrait for LiquidityPool { let config = get_config(&env); if offer_asset != config.token_a && offer_asset != config.token_b { - panic!("Trying to swap wrong asset. Aborting..") + panic_with_error!(env, ContractError::AssetNotInPool); } let pool_balance_a = utils::get_pool_balance_a(&env); diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index f0bdcbe18..c31509446 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -865,4 +865,3 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { &Some(spread), ); } - diff --git a/contracts/pool_stable/src/contract.rs b/contracts/pool_stable/src/contract.rs index 7d7d059b2..093fb7836 100644 --- a/contracts/pool_stable/src/contract.rs +++ b/contracts/pool_stable/src/contract.rs @@ -1,6 +1,9 @@ use phoenix::utils::LiquidityPoolInitInfo; -use soroban_sdk::{contract, contractimpl, contractmeta, log, Address, BytesN, Env, IntoVal}; +use soroban_sdk::{ + contract, contractimpl, contractmeta, log, panic_with_error, Address, BytesN, Env, IntoVal, +}; +use crate::contracterror::ContractError; use crate::storage::utils::{is_initialized, set_initialized}; use crate::storage::StableLiquidityPoolInfo; use crate::{ @@ -515,7 +518,7 @@ impl StableLiquidityPoolTrait for StableLiquidityPool { let config = get_config(&env); if offer_asset != config.token_a && offer_asset != config.token_b { - panic!("Trying to swap wrong asset. Aborting..") + panic_with_error!(env, ContractError::AssetNotInPool); } let pool_balance_a = utils::get_pool_balance_a(&env); diff --git a/contracts/pool_stable/src/contracterror.rs b/contracts/pool_stable/src/contracterror.rs new file mode 100644 index 000000000..f6423c84a --- /dev/null +++ b/contracts/pool_stable/src/contracterror.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 index 34a745c57..80764ba76 100644 --- a/contracts/pool_stable/src/lib.rs +++ b/contracts/pool_stable/src/lib.rs @@ -1,5 +1,6 @@ #![no_std] mod contract; +mod contracterror; mod math; mod storage; From e6c7149661fdf7e372643549a179e663e9845296 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Fri, 9 Feb 2024 17:22:53 +0200 Subject: [PATCH 05/13] removes unnecessary call to pool.provide_liquidity --- contracts/pool/src/tests/swap.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index c31509446..ac88e3461 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -842,17 +842,6 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { None, ); - token1.mint(&user1, &1_001_000); - token2.mint(&user1, &1_001_000); - pool.provide_liquidity( - &user1, - &Some(1_000_000), - &Some(1_000_000), - &Some(1_000_000), - &Some(1_000_000), - &None, - ); - // selling just one token with 1% max spread allowed let spread = 100i64; // 1% maximum spread allowed pool.swap( From 62eb11b581ac3f6276e7311ee7375375f51db98f Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Fri, 9 Feb 2024 19:18:53 +0200 Subject: [PATCH 06/13] Pool and Pool stable: removes redundant check for panic and adds it with if/else if/else --- contracts/pool/src/contract.rs | 23 +++++++++-------------- contracts/pool/src/tests/swap.rs | 2 +- contracts/pool_stable/src/contract.rs | 22 ++++++++++------------ 3 files changed, 20 insertions(+), 27 deletions(-) diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index 2e18cf6aa..f8c734ff8 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -583,16 +583,14 @@ impl LiquidityPoolTrait for LiquidityPool { fn simulate_swap(env: Env, offer_asset: Address, offer_amount: i128) -> SimulateSwapResponse { let config = get_config(&env); - if offer_asset != config.token_a && offer_asset != config.token_b { - panic_with_error!(env, ContractError::AssetNotInPool); - } - 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 { (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 compute_swap: ComputeSwap = compute_swap( @@ -622,16 +620,14 @@ impl LiquidityPoolTrait for LiquidityPool { ) -> SimulateReverseSwapResponse { let config = get_config(&env); - if ask_asset != config.token_a && ask_asset != config.token_b { - panic!("Trying to swap wrong asset. Aborting..") - } - 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 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 { + panic_with_error!(env, ContractError::AssetNotInPool); }; let (offer_amount, spread_amount, commission_amount) = compute_offer_amount( @@ -660,9 +656,6 @@ fn do_swap( max_spread: Option, ) -> i128 { let config = get_config(&env); - if offer_asset != config.token_a && offer_asset != config.token_b { - panic!("Trying to swap wrong asset. Aborting..") - } // FIXM: Disable Referral struct // if let Some(referral) = &referral { // if referral.fee > config.max_referral_bps { @@ -684,8 +677,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 diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index ac88e3461..a631e17ba 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -813,7 +813,7 @@ fn test_v_phx_vul_021_should_panic_when_max_spread_invalid_range(max_spread: Opt } #[test] -#[should_panic(expected = "Trying to swap wrong asset. Aborting..")] +#[should_panic(expected = "HostError: Error(Contract, #2)")] fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { let env = Env::default(); env.mock_all_auths(); diff --git a/contracts/pool_stable/src/contract.rs b/contracts/pool_stable/src/contract.rs index 093fb7836..3c8b33504 100644 --- a/contracts/pool_stable/src/contract.rs +++ b/contracts/pool_stable/src/contract.rs @@ -517,16 +517,14 @@ impl StableLiquidityPoolTrait for StableLiquidityPool { fn simulate_swap(env: Env, offer_asset: Address, offer_amount: i128) -> SimulateSwapResponse { let config = get_config(&env); - if offer_asset != config.token_a && offer_asset != config.token_b { - panic_with_error!(env, ContractError::AssetNotInPool); - } - 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 { (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( @@ -554,16 +552,14 @@ impl StableLiquidityPoolTrait for StableLiquidityPool { ) -> SimulateReverseSwapResponse { let config = get_config(&env); - if offer_asset != config.token_a && offer_asset != config.token_b { - panic!("Trying to swap wrong asset. Aborting..") - } - 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( @@ -610,8 +606,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( From 8dcce5fe3a4c3346cd752866d749b6be0c68b911 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Fri, 9 Feb 2024 19:27:20 +0200 Subject: [PATCH 07/13] Pool and Pool stable: renames contracterror.rs to error.rs; replaces panic! with panic_with_error! --- contracts/pool/src/contract.rs | 5 ++++- contracts/pool_stable/src/contract.rs | 2 +- contracts/pool_stable/src/{contracterror.rs => error.rs} | 0 contracts/pool_stable/src/lib.rs | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) rename contracts/pool_stable/src/{contracterror.rs => error.rs} (100%) diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index f8c734ff8..15b63c604 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -4,6 +4,9 @@ use soroban_sdk::{ use num_integer::Roots; +use crate::error::ContractError; +use crate::storage::utils::{is_initialized, set_initialized}; +use crate::storage::{ComputeSwap, LiquidityPoolInfo}; use crate::{ error::ContractError, stake_contract, @@ -809,7 +812,7 @@ fn split_deposit_based_on_pool_ratio( ) -> (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!("Trying an operation with wrong asset. Aborting..") + panic_with_error!(env, ContractError::AssetNotInPool); } // Validate the inputs diff --git a/contracts/pool_stable/src/contract.rs b/contracts/pool_stable/src/contract.rs index 3c8b33504..369a4b976 100644 --- a/contracts/pool_stable/src/contract.rs +++ b/contracts/pool_stable/src/contract.rs @@ -3,7 +3,7 @@ use soroban_sdk::{ contract, contractimpl, contractmeta, log, panic_with_error, Address, BytesN, Env, IntoVal, }; -use crate::contracterror::ContractError; +use crate::error::ContractError; use crate::storage::utils::{is_initialized, set_initialized}; use crate::storage::StableLiquidityPoolInfo; use crate::{ diff --git a/contracts/pool_stable/src/contracterror.rs b/contracts/pool_stable/src/error.rs similarity index 100% rename from contracts/pool_stable/src/contracterror.rs rename to contracts/pool_stable/src/error.rs diff --git a/contracts/pool_stable/src/lib.rs b/contracts/pool_stable/src/lib.rs index 80764ba76..8b5f22cdb 100644 --- a/contracts/pool_stable/src/lib.rs +++ b/contracts/pool_stable/src/lib.rs @@ -1,6 +1,6 @@ #![no_std] mod contract; -mod contracterror; +mod error; mod math; mod storage; From e0402e9073516f395ddb138174e07157236896af Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Fri, 9 Feb 2024 20:00:36 +0200 Subject: [PATCH 08/13] Pool: lints and fixes tests --- contracts/pool/src/contract.rs | 3 --- contracts/pool/src/error.rs | 1 + contracts/pool/src/tests/swap.rs | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index 15b63c604..3d92a2dfe 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -4,9 +4,6 @@ use soroban_sdk::{ use num_integer::Roots; -use crate::error::ContractError; -use crate::storage::utils::{is_initialized, set_initialized}; -use crate::storage::{ComputeSwap, LiquidityPoolInfo}; use crate::{ error::ContractError, stake_contract, diff --git a/contracts/pool/src/error.rs b/contracts/pool/src/error.rs index c8d68cede..d5d8579fb 100644 --- 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 index a631e17ba..83938d667 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -813,7 +813,7 @@ fn test_v_phx_vul_021_should_panic_when_max_spread_invalid_range(max_spread: Opt } #[test] -#[should_panic(expected = "HostError: Error(Contract, #2)")] +#[should_panic(expected = "HostError: Error(Contract, #13)")] fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { let env = Env::default(); env.mock_all_auths(); From 2373e5c4cab6da1893d253e15aaa78425e97a17b Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Mon, 12 Feb 2024 16:30:00 +0200 Subject: [PATCH 09/13] CHANGELOG.md: moves an entry from added to bugfixes --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a183322cf..e7be92730 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,20 +30,20 @@ and this project adheres to ## Added - Adds a new macro that validates the bps arguments value ([#199]) -- Pool and Pool stable: extend check if the swapped token is within pool ([#237]) [#199]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/199 -[#237]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/237 ## 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 From b45e3c2e08d877c23722291510858ce03126f571 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Mon, 12 Feb 2024 16:41:19 +0200 Subject: [PATCH 10/13] Pool: refactors test --- contracts/pool/src/tests/swap.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index 83938d667..200cd8cfd 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -831,19 +831,16 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { std::mem::swap(&mut admin1, &mut admin2); } let user1 = Address::generate(&env); - let swap_fees = 0i64; let pool = deploy_liquidity_pool_contract( &env, None, (&token1.address, &token2.address), - swap_fees, + 0i64, None, None, None, ); - - // selling just one token with 1% max spread allowed - let spread = 100i64; // 1% maximum spread allowed + // Swap fails because we provide incorrect token as offer token. pool.swap( &user1, // FIXM: Disable Referral struct @@ -851,6 +848,6 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { &bad_token.address, &1, &None, - &Some(spread), + &Some(100), ); } From 9424d6186113b771006b53efc054bf0e19871982 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Mon, 12 Feb 2024 17:06:47 +0200 Subject: [PATCH 11/13] Pool: adds more tests --- contracts/pool/src/tests/swap.rs | 74 ++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index 200cd8cfd..e4377251f 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -851,3 +851,77 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { &Some(100), ); } + + +#[test] +#[should_panic(expected = "HostError: Error(Contract, #13)")] +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, + ); + // Simulate swap fails because we provide incorrect token as offer token. + pool.simulate_swap( + // FIXM: Disable Referral struct + // &None::, + &bad_token.address, + &1, + ); +} + + +#[test] +#[should_panic(expected = "HostError: Error(Contract, #13)")] +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, + ); + // Simulate swap fails because we provide incorrect token as offer token. + pool.simulate_reverse_swap( + // FIXM: Disable Referral struct + // &None::, + &bad_token.address, + &1, + ); +} From 3f132bb7877de8c9d840a4f8a19816a6d06cd52f Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Mon, 12 Feb 2024 17:10:34 +0200 Subject: [PATCH 12/13] Pool: fmts --- contracts/pool/src/tests/swap.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index e4377251f..5f1923abb 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -852,7 +852,6 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { ); } - #[test] #[should_panic(expected = "HostError: Error(Contract, #13)")] fn test_v_phx_vul_017_should_panic_when_simulating_swap_for_non_existing_token_in_pool() { @@ -889,7 +888,6 @@ fn test_v_phx_vul_017_should_panic_when_simulating_swap_for_non_existing_token_i ); } - #[test] #[should_panic(expected = "HostError: Error(Contract, #13)")] fn test_v_phx_vul_017_should_panic_when_simulating_reverse_swap_for_non_existing_token_in_pool() { From 2e2288e2cfad32ebb5d593191775e01ee6bd3075 Mon Sep 17 00:00:00 2001 From: Kaloyan Gangov Date: Mon, 19 Feb 2024 11:12:38 +0200 Subject: [PATCH 13/13] pool: lints, adds comments according to PR, fixes conflicts, fixes failing tests --- contracts/pool/src/contract.rs | 2 ++ contracts/pool/src/tests/swap.rs | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/contracts/pool/src/contract.rs b/contracts/pool/src/contract.rs index 3d92a2dfe..a413b2568 100644 --- a/contracts/pool/src/contract.rs +++ b/contracts/pool/src/contract.rs @@ -590,6 +590,7 @@ impl LiquidityPoolTrait for LiquidityPool { } 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); }; @@ -627,6 +628,7 @@ impl LiquidityPoolTrait for LiquidityPool { } 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); }; diff --git a/contracts/pool/src/tests/swap.rs b/contracts/pool/src/tests/swap.rs index 5f1923abb..d5a2660ee 100644 --- a/contracts/pool/src/tests/swap.rs +++ b/contracts/pool/src/tests/swap.rs @@ -813,7 +813,7 @@ fn test_v_phx_vul_021_should_panic_when_max_spread_invalid_range(max_spread: Opt } #[test] -#[should_panic(expected = "HostError: Error(Contract, #13)")] +#[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(); @@ -839,6 +839,8 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { None, None, None, + Address::generate(&env), + Address::generate(&env), ); // Swap fails because we provide incorrect token as offer token. pool.swap( @@ -853,7 +855,7 @@ fn test_v_phx_vul_017_should_panic_when_swapping_non_existing_token_in_pool() { } #[test] -#[should_panic(expected = "HostError: Error(Contract, #13)")] +#[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(); @@ -878,6 +880,8 @@ fn test_v_phx_vul_017_should_panic_when_simulating_swap_for_non_existing_token_i None, None, None, + Address::generate(&env), + Address::generate(&env), ); // Simulate swap fails because we provide incorrect token as offer token. pool.simulate_swap( @@ -889,7 +893,7 @@ fn test_v_phx_vul_017_should_panic_when_simulating_swap_for_non_existing_token_i } #[test] -#[should_panic(expected = "HostError: Error(Contract, #13)")] +#[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(); @@ -914,6 +918,8 @@ fn test_v_phx_vul_017_should_panic_when_simulating_reverse_swap_for_non_existing None, None, None, + Address::generate(&env), + Address::generate(&env), ); // Simulate swap fails because we provide incorrect token as offer token. pool.simulate_reverse_swap(