Users can be forced to pay higher withdrawal fee when calling `remove_liquidity`

[c4-bot-10]

Lines of code

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

Vulnerability details

Issue Description

When users attempt to withdraw liquidity from the omnipool by calling the remove_liquidity function, a dynamic withdrawal fee is calculated based on current asset prices. This fee is calculated using the calculate_withdrawal_fee function from the omnipool math module, which takes into account the current pool price (spot price) and an external oracle price:

pub fn remove_liquidity(
    origin: OriginFor<T>,
    position_id: T::PositionItemId,
    amount: Balance,
) -> DispatchResult {
    ...

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

    ...
}

The calculate_withdrawal_fee function determines the withdrawal fee, which can vary from a minimum fee (min_fee) to a maximum fee (100% represented by FixedU128::one()):

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())
}

This means that in highly volatile markets, users may be required to pay a higher fee than expected when calling remove_liquidity (as the asset price might change from when the tx is submitted and when it's executed), potentially resulting in loss of funds and receiving less asset from their position than anticipated. Since the remove_liquidity function does not implement any protection against price movements (e.g., a minimum amount to be received by the user), users have no safeguard against increases in the withdrawal fee.

Impact

Users may receive fewer asset funds than expected when withdrawing liquidity due to being compelled to pay a higher dynamic withdrawal fee.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, it is recommended to modify the remove_liquidity function to include a max_fee_amount parameter provided by the users. This parameter will serve as a slippage protection, preventing users from paying a higher fee than anticipated.

pub fn remove_liquidity(
    origin: OriginFor<T>,
    position_id: T::PositionItemId,
    amount: Balance,
    max_fee_amount: FixedU128, // New parameter for maximum fee amount
) -> DispatchResult {
    ...

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

    ensure!(
        withdrawal_fee <= max_fee_amount,
        Error::<T>::ExcessiveWithdrawalFee
    );

    ...
}

By introducing the max_fee_amount parameter, users can limit their exposure to unexpected increases in withdrawal fees, providing them with greater control over their transactions.

Assessed type

Context

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory