omnipool: set_asset_tradable_state should ensure slot price if not far from oracle price when set an asset to safe withdraw mode

[c4-bot-10]

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:

1. The authority sets asset A to safe mode by disabling sell and buy operations by calling set_asset_tradable_state.
2. An attacker front-running the call above and buy/sell A, making spot/oracle price diff bigger.
3. 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.
4. (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]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[QiuhaoLi]

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]

OpenCoreCH marked the issue as not a duplicate

[c4-judge]

OpenCoreCH marked the issue as duplicate of #22