Merge pull request #199 from Phoenix-Protocol-Group/186-audit-missing…

…-incomplete-or-redundant-basis-point-range-checks

Phoenix: adds a new macro, that validates that the arguments which it takes are 0..10_000

CHANGELOG.md

@@ -13,6 +13,12 @@ and this project adheres to
 
 [#200]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/200
 
+## Added
+
+- Adds a new macro that validates the bps arguments value ([#199])
+
+[#199]: https://github.com/Phoenix-Protocol-Group/phoenix-contracts/pull/199
+
 ## [0.8.0] - 2024-01-17
 
 ## Changed

contracts/factory/src/contract.rs

@@ -8,6 +8,7 @@ use crate::{
     utils::deploy_lp_contract,
 };
 use phoenix::utils::{LiquidityPoolInitInfo, StakeInitInfo, TokenInitInfo};
+use phoenix::validate_bps;
 use soroban_sdk::{
     contract, contractimpl, contractmeta, log, Address, BytesN, Env, IntoVal, Symbol, Val, Vec,
 };
@@ -120,6 +121,13 @@ impl FactoryTrait for Factory {
             &lp_init_info.token_init_info.token_b,
         );
 
+        validate_bps!(
+            lp_init_info.swap_fee_bps,
+            lp_init_info.max_allowed_slippage_bps,
+            lp_init_info.max_allowed_spread_bps,
+            lp_init_info.max_referral_bps
+        );
+
         let init_fn: Symbol = Symbol::new(&env, "initialize");
         let init_fn_args: Vec<Val> =
             (stake_wasm_hash, token_wasm_hash, lp_init_info.clone()).into_val(&env);

contracts/pool/src/contract.rs

@@ -17,7 +17,7 @@ use crate::{
     token_contract,
 };
 use decimal::Decimal;
-use phoenix::{utils::is_approx_ratio, validate_int_parameters};
+use phoenix::{utils::is_approx_ratio, validate_bps, validate_int_parameters};
 
 // Metadata that is added on to the WASM custom section
 contractmeta!(
@@ -144,6 +144,13 @@ impl LiquidityPoolTrait for LiquidityPool {
         let token_init_info = lp_init_info.token_init_info;
         let stake_init_info = lp_init_info.stake_init_info;
 
+        validate_bps!(
+            swap_fee_bps,
+            max_allowed_slippage_bps,
+            max_allowed_spread_bps,
+            max_referral_bps
+        );
+
         set_initialized(&env);
 
         // Token info

contracts/pool/src/tests/liquidity.rs

@@ -542,7 +542,7 @@ fn provide_liqudity_single_asset_one_third_with_fees() {
 }
 
 #[test]
-#[should_panic(expected = "Pool: Initialize: Fees must be between 0 and 100%")]
+#[should_panic(expected = "The value 10001 is out of range. Must be between 0 and 10000 bps.")]
 fn provide_liqudity_too_high_fees() {
     let env = Env::default();
     env.mock_all_auths();

contracts/pool/src/tests/setup.rs

@@ -65,10 +65,6 @@ pub fn deploy_liquidity_pool_contract<'a>(
         stake_init_info,
     };
 
-    pool.initialize(
-        &stake_wasm_hash,
-        &token_wasm_hash,
-        &lp_init_info,
-    );
+    pool.initialize(&stake_wasm_hash, &token_wasm_hash, &lp_init_info);
     pool
 }

contracts/pool_stable/src/contract.rs

@@ -14,7 +14,7 @@ use crate::{
     token_contract,
 };
 use decimal::Decimal;
-use phoenix::validate_int_parameters;
+use phoenix::{validate_bps, validate_int_parameters};
 
 // Minimum amount of initial LP shares to mint
 const MINIMUM_LIQUIDITY_AMOUNT: i128 = 1000;
@@ -140,6 +140,11 @@ impl StableLiquidityPoolTrait for StableLiquidityPool {
         let token_init_info = lp_init_info.token_init_info;
         let stake_init_info = lp_init_info.stake_init_info;
 
+        validate_bps!(
+            swap_fee_bps,
+            max_allowed_slippage_bps,
+            max_allowed_spread_bps
+        );
         set_initialized(&env);
 
         // Token info

contracts/pool_stable/src/tests/liquidity.rs

@@ -542,7 +542,7 @@ fn provide_liqudity_single_asset_one_third_with_fees() {
 }
 
 #[test]
-#[should_panic(expected = "Pool: Initialize: Fees must be between 0 and 100%")]
+#[should_panic(expected = "The value 10001 is out of range. Must be between 0 and 10000 bps.")]
 fn provide_liqudity_too_high_fees() {
     let env = Env::default();
     env.mock_all_auths();

packages/phoenix/src/utils.rs

@@ -18,6 +18,21 @@ macro_rules! validate_int_parameters {
     };
 }
 
+// Validate all bps to be between the range 0..10_000
+#[macro_export]
+macro_rules! validate_bps {
+    ($($value:expr),+) => {
+        const MIN_BPS: i64 = 0;
+        const MAX_BPS: i64 = 10_000;
+        $(
+            // if $value < MIN_BPS || $value > MAX_BPS {
+            //     panic!("The value {} is out of range. Must be between {} and {} bps.", $value, MIN_BPS, MAX_BPS);
+            // }
+            assert!((MIN_BPS..=MAX_BPS).contains(&$value), "The value {} is out of range. Must be between {} and {} bps.", $value, MIN_BPS, MAX_BPS);
+        )+
+    }
+}
+
 pub fn is_approx_ratio(a: Decimal, b: Decimal, tolerance: Decimal) -> bool {
     let diff = (a - b).abs();
     diff <= tolerance
@@ -119,4 +134,21 @@ mod tests {
         let tolerance = Decimal::percent(3);
         assert!(!is_approx_ratio(a, b, tolerance));
     }
+
+    #[test]
+    #[should_panic(expected = "The value -1 is out of range. Must be between 0 and 10000 bps.")]
+    fn validate_bps_below_min() {
+        validate_bps!(-1, 300, 5_000, 8_534);
+    }
+
+    #[test]
+    #[should_panic(expected = "The value 10001 is out of range. Must be between 0 and 10000 bps.")]
+    fn validate_bps_above_max() {
+        validate_bps!(100, 10_001, 31_3134, 348);
+    }
+
+    #[test]
+    fn bps_valid_range() {
+        validate_bps!(0, 5_000, 7_500, 10_000);
+    }
 }