c4-bot-10 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L745

Vulnerability details

Impact

Users can lose all the withdrawn assets as fees when the asset is in safe withdraw mode.

Proof of Concept

In remove_liquidity, we will skip the spot/oracle price check if it's a safe mode withdraw (i.e. sell and buy are disabled).

            let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();
            // Skip price check if safe withdrawal - trading disabled.
            if !safe_withdrawal {
                T::PriceBarrier::ensure_price(
                    &who,
                    T::HubAssetId::get(),
                    asset_id,
                    EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
                )
                .map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;
            }

To prevent liquidity attacks, we apply a fee (max 100%) on the withdrawal action, which depends on the spot/oracle price diff.

            let ext_asset_price = T::ExternalPriceOracle::get_price(T::HubAssetId::get(), asset_id)?;

            if ext_asset_price.is_zero() {
                return Err(Error::<T>::InvalidOraclePrice.into());
            }
            let withdrawal_fee = hydra_dx_math::omnipool::calculate_withdrawal_fee(
                asset_state.price().ok_or(ArithmeticError::DivisionByZero)?,
                FixedU128::checked_from_rational(ext_asset_price.n, ext_asset_price.d)
                    .defensive_ok_or(Error::<T>::InvalidOraclePrice)?,
                T::MinWithdrawalFee::get(),
            );

It seems we assume if the asset is in safe mode, spot/oracle price diff should be minimal since the trade is disabled and other operations don't change the price.

However, in set_asset_tradable_state which can set an asset to the safe mode, we don't check if the spot/oracle diff is big (T::PriceBarrier::ensure_price), which can lead to:

The authority sets asset A to safe mode by disabling sell and buy operations by calling set_asset_tradable_state.
An attacker front-running the call above and buy/sell A, making spot/oracle price diff bigger.
A is in safe mode, Alice calls the remove liquidity function, and ensure_price is not triggered this time and Alice will suffer from a big fee.
(optional) Attacker does a reversed sell/buy call on asset A.

Tools Used

Manual Review.

Recommended Mitigation Steps

Call ensure_price in set_asset_tradable_state when we set an asset to the safe mode.

Assessed type

Context

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #158

c4-judge commented 6 months ago

OpenCoreCH marked the issue as satisfactory

QiuhaoLi commented 5 months ago

Hi @OpenCoreCH, thanks for the review. I think this issue is not duplicated to #93. #93 is about no minimum_amount_out for the liquidity removal function. But this issue is about the problem of setting the safe withdraw mode: set_asset_tradable_state does not ensure the slot price is not far from the Oracle price so it can be front-run by attackers. Later when the user calls the removal function, there will be no ensure_price call because it's in safe mode, and the user will suffer from max 100% fee loss. In contrast, #93 didn't mention the safe withdrawal attack mechanism and the impact is much smaller (~1%).

c4-judge commented 5 months ago

OpenCoreCH marked the issue as not a duplicate