Users can't withdraw liquidity if token value falls quickly

[c4-bot-8]

Lines of code

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

Vulnerability details

Bug Description

The HydraDX protocol implemented multiple safeguards against users manipulating the price of LRNA, as a result of the Critical-2304 found in the protocol. One of those safeguards is that in case of a token price deviating from the EMA oracle by more than 1% within a block, withdrawals of this token are halted.

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)?;
}

The EMA oracle calculates an exponential moving average of the past prices in Stableswap / Omnipool. Unfortunately, this fix results in a new issue if the price of a token falls really fast, at a faster rate than 1% per block. In that case, the liquidity of this token will not be withdrawable until the price has started to bottom out. The users that have deposited liquidity of the falling token are forced to endure the full impermanent loss.

Impact

This issue results in liquidity providers being forced to endure the impermanent loss of a token in the pool.

Proof of Concept

The following testcase showcases the functionality

#[test]
fn remove_liquidity_should_not_work_on_quick_fall() {
    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (Omnipool::protocol_account(), DAI, 1000 * ONE),
            (Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
            (LP2, 1_000, 2000 * ONE),
            (LP1, 1_000, 5000 * ONE),
        ])
        .with_registered_asset(1_000)
        .with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
        .with_token(1_000, FixedU128::from_float(0.65), LP1, 2000 * ONE)
        .with_max_allowed_price_difference(Permill::from_percent(1))
        .build()
        .execute_with(|| {
            let current_position_id = <NextPositionId<Test>>::get();

            //Adjustment by 3%
            EXT_PRICE_ADJUSTMENT.with(|v| {
                *v.borrow_mut() = (3, 100, true);
            });

            //LP1 can't withdraw his liquidity as the price has fallen too quick
            assert_noop!(
                Omnipool::remove_liquidity(RuntimeOrigin::signed(LP1), current_position_id-1, 200 * ONE,),
                Error::<Test>::PriceDifferenceTooHigh
            );
        });
}

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by implementing a different functionality for preventing price manipulations. An example could be to include an external oracle like Chainlink. If the price from Chainlink is also falling very quickly, users should be able to withdraw their liquidity to not be forced to endure impermanent loss.

Assessed type

DoS

[0xRobocop]

Seem like a design decision.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

By design, the issue states that a mechanism intended to prevent withdrawals when the price differs too much prevents withdrawals when the price differs too much.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid