Users can lose up to 100% of their funds when removing liquidity

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

There is no slippage protection when removing liquidity. An attacker can manipulate the spot price of an asset to increase the withdrawal fees (up to 100%) for other users before they withdraw their funds.

This will result in a partial or total loss of funds, depending on how much the spot price is increased in comparison to the oracle price.

Proof of Concept

When removing liquidity, there are some checks to ensure that the price delta between the oracle and the spot price falls within 1%. This also normally serves as a "slippage" protection, as the transaction will fail when these prices differ too much.

However, when trading is disabled for an asset (i.e. safe_withdrawal = true), the price difference check is skipped:

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)?;
}
let ext_asset_price = T::ExternalPriceOracle::get_price(T::HubAssetId::get(), asset_id)?;

Nevertheless, the withdrawal_fee will be calculated using the delta between oracle price and spot price:

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(),
);

This is the function that will calculate the withdrawal fee:

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;
    }
    price_diff.div(oracle_price).clamp(min_fee, FixedU128::one())
}

Which is the price delta % between spot and oracle price, capped between min_fee and 100%.

Let's assume that an attacker frontruns the transaction to manipulate the spot price (even if difficult, this is possible on substrate).

If the new spot price is at least twice the amount of the oracle price, the withdrawal fee for the victim will be 100% of their liquidity, leading to a total loss of funds.

Afterward, the liquidity received by the user will be calculated as delta_reserve:

let fee_complement = FixedU128::one().saturating_sub(withdrawal_fee); //@audit fee_complement is zero

// Apply withdrawal fee
let delta_reserve = fee_complement.checked_mul_int(delta_reserve)?; //@audit zero multiplied by anything is zero
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)?;

Some(LiquidityStateChange {
	asset: AssetStateChange {
		delta_reserve: Decrease(delta_reserve), //@audit delta_reserve is zero
		delta_hub_reserve: Decrease(delta_hub_reserve),
		delta_shares: Decrease(delta_shares),
		delta_protocol_shares: Increase(delta_b),
	},
	delta_imbalance: Increase(delta_imbalance),
	lp_hub_amount: hub_transferred,
	delta_position_reserve: Decrease(delta_position_amount),
	delta_position_shares: Decrease(shares_removed),
})

Finally, the user receives zero funds for their shares:

T::Currency::transfer(
	asset_id,
	&Self::protocol_account(),
	&who,
	*state_changes.asset.delta_reserve, //@audit zero
)?;

Recommended Mitigation Steps

Consider implementing some slippage protection when removing liquidity.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)