c4-bot-1 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

When the authority calls withdraw_protocol_liquidity to remove protocol liquidity, it's vulnerable to sandwich attacks.

Proof of Concept

In withdraw_protocol_liquidity, we didn't make sure the spot price was not too far from the EMA oracle price. Which can lead to:

The authority calls withdraw_protocol_liquidity.
Attakcers call buy or sell, making the spot price and EMA price diff bigger than the max allowed one.
The withdrawal is executed, for example, the authority is supposed to get all the asset tokens, but now he can only get some asset tokens and LRNA, which in all worth less than expected.

Tools Used

Manual Review.

Recommended Mitigation Steps

Like remote_liquidity, we should ensure spot/oracle price diff is not bigger than the max allowed one:

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

Assessed type

MEV

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 but #84:

Same problem: 93 is about no minimal out amount in remove_liquidity. This issue is about withdraw_protocol_liquidity slippage for the admin, which is the same issue as 84.
Same Root Cause: 84 pointed out that compared to remove_liquidity, withdraw_protocol_liquidity lacks the ensure_price check (check the "Proof of Concept"), which is the same root cause pointed out in this issue (check the "Recommended Mitigation Steps").
(Not important for this judge) Although 84 additionally pointed out the add a safe_withdrawalparameter, this is not the root cause (ensure_price). And the set_asset_tradable_state doesn't check if the current spot price is far from the oracle price (vulnerable to front-run, #174), so using ensure_price directly would be better..

According to 1 and 2, I believe this issue is duplicated to #84.

c4-judge commented 5 months ago

OpenCoreCH marked the issue as not a duplicate