Merge pull request #5 from solendprotocol/audit_fixes

Audit fixes

Author: 0xripleys

contracts/sources/fees.move

@@ -2,8 +2,13 @@ module liquid_staking::fees {
     use sui::bag::{Self, Bag};
     use std::u64::max;
 
+    // Errors
     const EInvalidFeeConfig: u64 = 0;
 
+    // Constants
+    const MAX_BPS: u64 = 10_000;
+    const MAX_REDEEM_FEE_BPS: u64 = 500; // 5%
+
     public struct FeeConfig has store {
         sui_mint_fee_bps: u64,
         staked_sui_mint_fee_bps: u64, // unused
@@ -127,12 +132,12 @@ module liquid_staking::fees {
     // This is because having a 0 fee LST is useful in certain cases where mint/redemption can only be done by 
     // a single party. It is up to the pool creator to ensure that the fees are set correctly.
     fun validate_fees(fees: &FeeConfig) {
-        assert!(fees.sui_mint_fee_bps <= 10_000, EInvalidFeeConfig);
-        assert!(fees.staked_sui_mint_fee_bps <= 10_000, EInvalidFeeConfig);
-        assert!(fees.redeem_fee_bps <= 10_000, EInvalidFeeConfig);
-        assert!(fees.staked_sui_redeem_fee_bps <= 10_000, EInvalidFeeConfig);
-        assert!(fees.spread_fee_bps <= 10_000, EInvalidFeeConfig);
-        assert!(fees.custom_redeem_fee_bps <= 10_000, EInvalidFeeConfig);
+        assert!(fees.sui_mint_fee_bps <= MAX_BPS, EInvalidFeeConfig);
+        assert!(fees.staked_sui_mint_fee_bps <= MAX_BPS, EInvalidFeeConfig);
+        assert!(fees.redeem_fee_bps <= MAX_REDEEM_FEE_BPS, EInvalidFeeConfig);
+        assert!(fees.staked_sui_redeem_fee_bps <= MAX_BPS, EInvalidFeeConfig);
+        assert!(fees.spread_fee_bps <= MAX_BPS, EInvalidFeeConfig);
+        assert!(fees.custom_redeem_fee_bps <= MAX_BPS, EInvalidFeeConfig);
     }
 
     public(package) fun calculate_mint_fee(self: &FeeConfig, sui_amount: u64): u64 {
@@ -161,7 +166,7 @@ module liquid_staking::fees {
         let fees = FeeConfig {
             sui_mint_fee_bps: 10_000,
             staked_sui_mint_fee_bps: 10_000,
-            redeem_fee_bps: 10_000,
+            redeem_fee_bps: 500,
             staked_sui_redeem_fee_bps: 10_000,
             spread_fee_bps: 10_000,
             custom_redeem_fee_bps: 10_000,

contracts/sources/liquid_staking.move

@@ -20,6 +20,8 @@ module liquid_staking::liquid_staking {
     const EMintInvariantViolated: u64 = 1;
     const ERedeemInvariantViolated: u64 = 2;
     const EValidatorNotFound: u64 = 3;
+    const EZeroLstSupply: u64 = 4;
+    const EZeroLstMinted: u64 = 5;
 
     /* Constants */
     const CURRENT_VERSION: u16 = 1;
@@ -159,6 +161,16 @@ module liquid_staking::liquid_staking {
         storage.join_to_sui_pool(sui.into_balance());
 
         assert!(lst_treasury_cap.total_supply() > 0 && storage.total_sui_supply() > 0, EInvalidLstCreation);
+
+        // make sure the lst ratio is in a sane range:
+        let total_sui_supply = (storage.total_sui_supply() as u128);
+        let total_lst_supply = (lst_treasury_cap.total_supply() as u128);
+        assert!(
+            (total_sui_supply >= total_lst_supply)
+            && (total_sui_supply <= 2 * total_lst_supply), // total_sui_supply / total_lst_supply <= 2
+            EInvalidLstCreation
+        );
+
         create_lst_with_storage(
             fee_config,
             lst_treasury_cap,
@@ -215,6 +227,7 @@ module liquid_staking::liquid_staking {
         self.fees.join(sui_balance.split(mint_fee_amount));
 
         let lst_mint_amount = self.sui_amount_to_lst_amount(sui_balance.value());
+        assert!(lst_mint_amount > 0, EZeroLstMinted);
 
         emit_event(MintEvent {
             typename: type_name::get<P>(),
@@ -228,7 +241,8 @@ module liquid_staking::liquid_staking {
         // invariant: lst_out / sui_in <= old_lst_supply / old_sui_supply
         // -> lst_out * old_sui_supply <= sui_in * old_lst_supply
         assert!(
-            (lst.value() as u128) * old_sui_supply <= (sui_balance.value() as u128) * old_lst_supply,
+            ((lst.value() as u128) * old_sui_supply <= (sui_balance.value() as u128) * old_lst_supply)
+            || (old_sui_supply > 0 && old_lst_supply == 0), // special case
             EMintInvariantViolated
         );
 
@@ -324,7 +338,7 @@ module liquid_staking::liquid_staking {
         _: &AdminCap<P>,
         system_state: &mut SuiSystemState,
         validator_address: address,
-        max_sui_amount: u64,
+        target_unstake_sui_amount: u64,
         ctx: &mut TxContext
     ): u64 {
         self.refresh(system_state, ctx);
@@ -335,7 +349,7 @@ module liquid_staking::liquid_staking {
         let sui_amount = self.storage.unstake_approx_n_sui_from_validator(
             system_state,
             validator_index,
-            max_sui_amount,
+            target_unstake_sui_amount,
             ctx
         );
 
@@ -449,7 +463,8 @@ module liquid_staking::liquid_staking {
         let total_sui_supply = self.total_sui_supply();
         let total_lst_supply = self.total_lst_supply();
 
-        // div by zero case should never happen
+        assert!(total_lst_supply > 0, EZeroLstSupply);
+
         let sui_amount = (total_sui_supply as u128)
             * (lst_amount as u128) 
             / (total_lst_supply as u128);

contracts/sources/storage.move

@@ -7,9 +7,15 @@ module liquid_staking::storage {
     use sui::bag::{Self, Bag};
 
     /* Errors */
+    const ENotEnoughSuiInSuiPool: u64 = 0;
+    const ENotActiveValidator: u64 = 1;
+    const ETooManyValidators: u64 = 2;
+    const EValidatorAlreadyExists: u64 = 3;
 
     /* Constants */
     const MIN_STAKE_THRESHOLD: u64 = 1_000_000_000;
+    const MAX_SUI_SUPPLY: u64 = 10_000_000_000 * 1_000_000_000;
+    const MAX_VALIDATORS: u64 = 50;
 
     /// The Storage struct holds all stake for the LST.
     public struct Storage has store {
@@ -129,32 +135,41 @@ module liquid_staking::storage {
             return false
         };
 
+        let active_validator_addresses = system_state.active_validator_addresses();
+
         let mut i = self.validator_infos.length();
         while (i > 0) {
             i = i - 1;
 
-            // update pool token exchange rates
-            let validator_info = &mut self.validator_infos[i];
+            // if validator is inactive, withdraw all stake.
+            if (!active_validator_addresses.contains(&self.validator_infos[i].validator_address)) {
+                // technically this is using a stale exchange rate, but it doesn't matter because we're unstaking everything.
+                // this is done before fetching the exchange rate because i don't want the function to abort if an epoch is skipped.
+                self.unstake_approx_n_sui_from_validator(system_state, i, MAX_SUI_SUPPLY, ctx);
+            };
 
-            let exchange_rates = system_state.pool_exchange_rates(&validator_info.staking_pool_id);
+            if (self.validator_infos[i].is_empty()) {
+                let ValidatorInfo { active_stake, inactive_stake, extra_fields, .. } = self.validator_infos.remove(i);
+                active_stake.destroy_none();
+                inactive_stake.destroy_none();
+                extra_fields.destroy_empty();
+
+                continue
+            };
+
+            // update pool token exchange rates
+            let exchange_rates = system_state.pool_exchange_rates(&self.validator_infos[i].staking_pool_id);
             let latest_exchange_rate = exchange_rates.borrow(ctx.epoch());
 
-            validator_info.exchange_rate = *latest_exchange_rate;
+            self.validator_infos[i].exchange_rate = *latest_exchange_rate;
+            self.refresh_validator_info(i);
 
-            if (validator_info.inactive_stake.is_some()) {
+            if (self.validator_infos[i].inactive_stake.is_some()) {
                 let inactive_stake = self.take_from_inactive_stake(i);
                 let fungible_staked_sui = system_state.convert_to_fungible_staked_sui(inactive_stake, ctx);
                 self.join_fungible_staked_sui_to_validator(i, fungible_staked_sui);
             };
 
-            refresh_validator_info(self, i);
-
-            if (self.validator_infos[i].is_empty()) {
-                let ValidatorInfo { active_stake, inactive_stake, extra_fields, .. } = self.validator_infos.remove(i);
-                active_stake.destroy_none();
-                inactive_stake.destroy_none();
-                extra_fields.destroy_empty();
-            };
         };
 
         self.last_refresh_epoch = ctx.epoch();
@@ -373,7 +388,7 @@ module liquid_staking::storage {
         let target_unstake_sui_amount = max(target_unstake_sui_amount, MIN_STAKE_THRESHOLD);
 
         let staked_sui_amount = validator_info.inactive_stake.borrow().staked_sui_amount();
-        let staked_sui = if (staked_sui_amount <= target_unstake_sui_amount + MIN_STAKE_THRESHOLD) {
+        let staked_sui = if (staked_sui_amount < target_unstake_sui_amount + MIN_STAKE_THRESHOLD) {
             self.take_from_inactive_stake(validator_index)
         } 
         else {
@@ -424,6 +439,7 @@ module liquid_staking::storage {
             };
         };
 
+        assert!(self.sui_pool.value() >= max_sui_amount_out, ENotEnoughSuiInSuiPool);
         self.split_from_sui_pool(max_sui_amount_out)
     }
 
@@ -495,16 +511,31 @@ module liquid_staking::storage {
         staking_pool_id: ID,
         ctx: &mut TxContext
     ): u64 {
+        let mut current_validator_addresses = vector[];
+
         let mut i = 0;
         while (i < self.validator_infos.length()) {
             if (self.validator_infos[i].staking_pool_id == staking_pool_id) {
                 return i
             };
 
+            current_validator_addresses.push_back(self.validator_infos[i].validator_address);
             i = i + 1;
         };
 
         let validator_address = system_state.validator_address_by_pool_id(&staking_pool_id);
+
+        assert!(
+            !current_validator_addresses.contains(&validator_address),
+            EValidatorAlreadyExists
+        );
+
+        let active_validator_addresses = system_state.active_validator_addresses();
+        assert!(
+            active_validator_addresses.contains(&validator_address),
+            ENotActiveValidator
+        );
+
         let exchange_rates = system_state.pool_exchange_rates(&staking_pool_id);
         let latest_exchange_rate = exchange_rates.borrow(ctx.epoch());
 
@@ -518,6 +549,8 @@ module liquid_staking::storage {
             extra_fields: bag::new(ctx)
         });
 
+        assert!(self.validator_infos.length() <= MAX_VALIDATORS, ETooManyValidators);
+
         i
     }
 

contracts/tests/liquid_staking_tests.move

@@ -92,7 +92,7 @@ module liquid_staking::liquid_staking_tests {
 
         // Create a treasury cap with non-zero coins
         let mut treasury_cap = coin::create_treasury_cap_for_testing<TEST>(scenario.ctx());
-        let coins = treasury_cap.mint(1000 * MIST_PER_SUI, scenario.ctx());
+        let coins = treasury_cap.mint(200 * MIST_PER_SUI, scenario.ctx());
 
         let (admin_cap, lst_info) = create_lst_with_stake<TEST>(
             &mut system_state,
@@ -106,7 +106,7 @@ module liquid_staking::liquid_staking_tests {
             scenario.ctx()
         );
 
-        assert!(lst_info.total_lst_supply() == 1000 * MIST_PER_SUI, 0);
+        assert!(lst_info.total_lst_supply() == 200 * MIST_PER_SUI, 0);
         assert!(lst_info.storage().total_sui_supply() == 200 * MIST_PER_SUI, 0);
 
         test_scenario::return_shared(system_state);
@@ -217,6 +217,72 @@ module liquid_staking::liquid_staking_tests {
         scenario.end();
     }
 
+    #[test]
+    #[expected_failure(abort_code = 0, location = liquid_staking::liquid_staking)]
+    fun test_create_lst_with_stake_fail_3() {
+        let mut scenario = test_scenario::begin(@0x0);
+
+        setup_sui_system(&mut scenario, vector[100, 100]);
+
+        let mut system_state = scenario.take_shared<SuiSystemState>();
+
+        let mut treasury_cap = coin::create_treasury_cap_for_testing(scenario.ctx());
+        let coins = treasury_cap.mint(1000 * MIST_PER_SUI, scenario.ctx());
+
+        let (admin_cap, lst_info) = create_lst_with_stake<TEST>(
+            &mut system_state,
+            fees::new_builder(scenario.ctx())
+                .set_sui_mint_fee_bps(100)
+                .set_redeem_fee_bps(100)
+                .to_fee_config(),
+            treasury_cap,
+            vector::empty(),
+            coin::mint_for_testing(1000  * MIST_PER_SUI - 1, scenario.ctx()),
+            scenario.ctx()
+        );
+
+        test_scenario::return_shared(system_state);
+
+        sui::test_utils::destroy(admin_cap);
+        sui::test_utils::destroy(lst_info);
+        sui::test_utils::destroy(coins);
+
+        scenario.end();
+    }
+
+    #[test]
+    #[expected_failure(abort_code = 0, location = liquid_staking::liquid_staking)]
+    fun test_create_lst_with_stake_fail_4() {
+        let mut scenario = test_scenario::begin(@0x0);
+
+        setup_sui_system(&mut scenario, vector[100, 100]);
+
+        let mut system_state = scenario.take_shared<SuiSystemState>();
+
+        let mut treasury_cap = coin::create_treasury_cap_for_testing(scenario.ctx());
+        let coins = treasury_cap.mint(1000 * MIST_PER_SUI, scenario.ctx());
+
+        let (admin_cap, lst_info) = create_lst_with_stake<TEST>(
+            &mut system_state,
+            fees::new_builder(scenario.ctx())
+                .set_sui_mint_fee_bps(100)
+                .set_redeem_fee_bps(100)
+                .to_fee_config(),
+            treasury_cap,
+            vector::empty(),
+            coin::mint_for_testing(2000  * MIST_PER_SUI + 1, scenario.ctx()),
+            scenario.ctx()
+        );
+
+        test_scenario::return_shared(system_state);
+
+        sui::test_utils::destroy(admin_cap);
+        sui::test_utils::destroy(lst_info);
+        sui::test_utils::destroy(coins);
+
+        scenario.end();
+    }
+
     #[test]
     fun test_mint_and_redeem() {
         let mut scenario = test_scenario::begin(@0x0);
@@ -668,7 +734,7 @@ module liquid_staking::liquid_staking_tests {
 
         let staked_sui = stake_with(0, std::u64::max(mint_amount / MIST_PER_SUI, 1), &mut scenario);
         let mut treasury_cap = coin::create_treasury_cap_for_testing<TEST>(scenario.ctx());
-        let lst = treasury_cap.mint(100 * MIST_PER_SUI, scenario.ctx());
+        let lst = treasury_cap.mint(mint_amount / MIST_PER_SUI * MIST_PER_SUI, scenario.ctx());
 
         advance_epoch_with_reward_amounts(0, 0, &mut scenario);