a huge loss of funds for all the users who try to remove liquidity after swapping got disabled at manipulated price .

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1330-L1360
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L743
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L759-L764

Vulnerability details

Impact

this vulnerability will lead to huge loss of funds for liquidity providers that want to withdraw their liquidity if the safe withdrawal is enabled .

• the loss of funds can be 100% of the liquidity provider`s shares .

Proof of Concept

Normal Scenario of manipulating price and disabling removing or adding liquidity

If the price of certain asset got manipulated , there is an ensure function exist in the remove_liquidity() here and add_liquidity()here , so the function should revert in case of the price of an asset got manipulated .

                        T::PriceBarrier::ensure_price(
                                &who,
                                T::HubAssetId::get(),
                                asset,
                                EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
                        )
                        .map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;

This ensure_price() function checks that the difference between spot price and oracle price is not too high , so it has critical role to prevent the profitability from this manipulation .

There is also another security parameter which is the Tradability state which can prevent removing or adding liquidity .

And there is withdrawal_fee which is used to make manipulating price not profitable , and it can prevent the attacker from getting any of assets if the price difference is too high .

Important assumption

the assumption is that the withdrawal can be done safely without checking the price difference because the swapping of this asset got disabled so the price is stable .
as shown here

                        let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();

        pub(crate) fn is_safe_withdrawal(&self) -> bool {
                *self == Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY || *self == Tradability::REMOVE_LIQUIDITY
        }
}

Edge case

Due to the fact that there is not limitation on setting tradability states to any asset except the hub_asset , the tradability state can be set to prevent swapping on asset at manipulated price , by make the tradability state only contains remove and add liquidity flags , when the difference between spot price and the oracle price is too high .
In such case the remove_liquidity() function will not revert with price error because the function ensure-price() will not work , but it will pass and the withdrawal_fee will be equal to 1 .
So 100% of the liquidity to be removed will be taken from the user as fees and will be distributed on the other liquidity providers .

how this vulnerability can be applied :

1. the price of asset got manipulated and the difference between spot and oracle price is too high
2. the tardability state of the asset has been set to remove and add liquidity only (buying and selling are disabled )
3. any user trying to remove his liquidity will be taken as fees and there is no asset will be transferred to the user and the transaction will not revert , since there is no slippage(safty) parameter can be set by the user , to ensure that amount comes from this transaction is equal to what is expected .

the normal Scenario here is that the remove_liquidity function should revert instead of taking all user assets as withdrawal_fee
the code that calculate the withdrawal fee is

pub fn calculate_withdrawal_fee(
        spot_price: FixedU128,
        oracle_price: FixedU128,
        min_withdrawal_fee: Permill,
) -> FixedU128 {
        let price_diff = if oracle_price <= spot_price {
                spot_price.saturating_sub(oracle_price)
        } else {
                oracle_price.saturating_sub(spot_price)
        };


        let min_fee: FixedU128 = min_withdrawal_fee.into();
        debug_assert!(min_fee <= FixedU128::one());


        if oracle_price.is_zero() {
                return min_fee;
        }

        // fee can be set to 100% 
  ->    price_diff.div(oracle_price).clamp(min_fee, FixedU128::one())
}

the delta assets that send to the user will be zero in case that withdrawal_fee is 1

        // fee_complement = 0 ; 
        let fee_complement = FixedU128::one().saturating_sub(withdrawal_fee);


        // Apply withdrawal fee
        let delta_reserve = fee_complement.checked_mul_int(delta_reserve)?;
        let delta_hub_reserve = fee_complement.checked_mul_int(delta_hub_reserve)?;
        let hub_transferred = fee_complement.checked_mul_int(hub_transferred)?;


        let delta_imbalance = calculate_delta_imbalance(delta_hub_reserve, imbalance, total_hub_reserve)?; 

Tools Used

vs code and manual review

Recommended Mitigation Steps

this vulnerability can be mitigated by only one step :

• to check that the price is in the allowed Range before disabling the swapping and allow remove and add liquidity on any asset , this mitigation will make sure that the safe_withdrawal is set to true except if the price in the Range so the price is actually stable and safe to withdraw liquidity on this price .

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1330-L1361

consider modifying set_asset_tradable_state() function to ensure that if the state is set to preventing swapping , then ensure the price

                pub fn set_asset_tradable_state(
                        origin: OriginFor<T>,
                        asset_id: T::AssetId,
                        state: Tradability,
                ) -> DispatchResult {
                        T::TechnicalOrigin::ensure_origin(origin)?;


                        if asset_id == T::HubAssetId::get() {
                                // Atm omnipool does not allow adding/removing liquidity of hub asset.
                                // Although BUY is not supported yet, we can allow the new state to be set to SELL/BUY.
                                ensure!(
                                        !state.contains(Tradability::ADD_LIQUIDITY) && !state.contains(Tradability::REMOVE_LIQUIDITY),
                                        Error::<T>::InvalidHubAssetTradableState
                                );


                                HubAssetTradability::<T>::mutate(|value| -> DispatchResult {
                                        *value = state;
                                        Self::deposit_event(Event::TradableStateUpdated { asset_id, state });
                                        Ok(())
                                })
                        } else {

                                Assets::<T>::try_mutate(asset_id, |maybe_asset| -> DispatchResult {
                                        let asset_state = maybe_asset.as_mut().ok_or(Error::<T>::AssetNotFound)?;
+ if (state == Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY || state == Tradability::REMOVE_LIQUIDITY){
+
+      T::PriceBarrier::ensure_price(
+					&who,
+					T::HubAssetId::get(),
+					asset_id,
+					EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
+				)
+				.map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;}

                                        asset_state.tradable = state;
                                        Self::deposit_event(Event::TradableStateUpdated { asset_id, state });


                                        Ok(())
                                })
                        }
                }

Assessed type

Invalid Validation

[0xRobocop]

Seems to have too many hand wavy hypotheticals

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[OpenCoreCH]

Intended behaviour / design that this check is not performed in this state which can only be set by the AuthorityOrigin, downgrading to QA

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[Frankcastleauditor]

Hi @OpenCoreCH
this issue should be marked as a duplicate of the issues number #135 , and #174 , and they are all not a duplicate of the issue number #93 , since it does not have the same impact or the same location of this finding , but this finding and 135 , and 174 they have the same location and the same impact :

1. this finding points to the vulnerable function set_asset_tradable_state , because it does not check that the price of the oracle is not too far from the spot price before activate the safe mode so it can be front-run by attackers.
2. the impact if the 100% of the liquidity withdrawn by the user will be taken as the withdrawal_fee , the impact of the issue number 93 is just a 1% due to the absence of the slippage parameter .

[c4-judge]

This previously downgraded issue has been upgraded by OpenCoreCH

[OpenCoreCH]

This issue, #135, and #174 all deal with the withdrawal fees for safe withdrawals. #135 does so from a different perspective (and suggests a different solution), but the underlying issue is still the same.

The issue demonstrates that there can be edge cases where a very high fee is charged, I am therefore upgrading it to a medium.

[c4-judge]

OpenCoreCH marked the issue as primary issue

[c4-judge]

OpenCoreCH marked the issue as selected for report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[c4-sponsor]

enthusiastmartin (sponsor) acknowledged