omnipool: MaxOutRatio protections in buy should consider asset fees -- use amount_without_fee instead of asset_state.reserve

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

More assets than asset.reverse / MaxOutRatio are taken out when buy_asset_for_hub_asset or buy is called.

Proof of Concept

In omnipool, we apply asset fees on trading. Especially, during trading, the asset reverse will be split into two parts: 1. Amount we can take out. 2. The asset fees. For example, in pub fn calculate_buy_for_hub_asset_state_changes:

/// Calculate delta changes of a buy trade where asset_in is Hub Asset
pub fn calculate_buy_for_hub_asset_state_changes(
    asset_out_state: &AssetReserveState<Balance>,
    asset_out_amount: Balance,
    asset_fee: Permill,
    imbalance: I129<Balance>,
    total_hub_reserve: Balance,
) -> Option<HubTradeStateChange<Balance>> {
    let reserve_no_fee = amount_without_fee(asset_out_state.reserve, asset_fee)?;
    let hub_denominator = reserve_no_fee.checked_sub(asset_out_amount)?;

    let (hub_reserve_hp, amount_hp, hub_denominator_hp) =
        to_u256!(asset_out_state.hub_reserve, asset_out_amount, hub_denominator);

    let delta_hub_reserve_hp = hub_reserve_hp.checked_mul(amount_hp).and_then(|v| {
        v.checked_div(hub_denominator_hp)
            .and_then(|v| v.checked_add(U256::one()))
    })?;

Let's say the asset.reserve is 100, and the asset fee is 20%, then the reserve will be split as 80:20. If asset_out_amount is 10, then the fee will be (10/80) * 20 + 1 = 3.

To prevent liquidity attacks, we restrict the max ration of input amount in buy functions:

            ensure!(
                amount
                    <= asset_out_state
                        .reserve
                        .checked_div(T::MaxOutRatio::get())
                        .ok_or(ArithmeticError::DivisionByZero)?, // Note: Let's be safe. this can only fail if MaxOutRatio is zero.
                Error::<T>::MaxOutRatioExceeded
            );

However, the amount provided by the user is the final amount taken out from the pool, not including the fees. In the example above, the amount will be 10, and the actual output ratio will be 10/80 (or (10+3)/100), which is bigger than what we calculate as 10/100. So more ratio assets are taken out.

Tools Used

Manual review.

Recommended Mitigation Steps

let reserve_no_fee = amount_without_fee(asset_out_state.reserve, asset_fee)?;
        ensure!(
            amount
                <= reserve_no_fee
                    .checked_div(T::MaxOutRatio::get())
                    .ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxInRatio is zero.
            Error::<T>::MaxOutRatioExceeded
        );

Assessed type

Math

[0xRobocop]

The report is of low quality, the mitigation it is the same code snippet from the one above.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[QiuhaoLi]

The report is of low quality, the mitigation it is the same code snippet from the one above.

@0xRobocop FYI, asset_out_state.reserve --> reserve_no_fee.

[OpenCoreCH]

Based on the definition of MaxOutRatio (Max fraction of asset reserve to buy in single transaction), it seems intended that the fees are not taken into account. Taking them into account would be an alternative design, but the report does not show why the current design / definition of the ratio leads to a problem -> QA

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-b

[QiuhaoLi]

Based on the definition of MaxOutRatio (Max fraction of asset reserve to buy in single transaction), it seems intended that the fees are not taken into account.

Hi @OpenCoreCH, thanks for the review. Yes, this is where the problem lies. I should have explained it more clearly: When we use MaxOutRatio, the fees are intended to not be taken into account, but we used it with asset_out_state.reserve, which includes the fees, leading to situations where more assets than MaxOutRatio are taken out of the pool.

Give an example: asset.reserve is 100, MaxOutRation is 40%, and the asset fee is 20%, then the reserve will be split as 80:20 (check calculate_buy_for_hub_asset_state_changes, it will split reserve as no_fees and fees). In the current situation, the max allowed amount out will be 10040% = 40, then the fee is calculated as (40/80) 20 + 1 = 11. In the end, the assets taken out are (40/80) or (51/100) ~50%, which is bigger than 40% both for the assets not including the fees or as a whole. This can also lead to DoS if MaxOutRation is bigger than the asset fee (not enough fees during the removal, but the parameters are controlled by the authority).

In short, the root cause is amount provided by the user is the final amount taken out, but we apply it with MaxOutRatio on the asset.reserve which includes the fees, so more than MaxOutRatio assets can be taken out of the pool in the end.

[OpenCoreCH]

Ok, I think I see what you mean. Nevertheless keeping it as QA, as these differences are very small for a sufficiently large pool and even if more than MaxOutRatio assets can be taken out, it is not clear / shown what the impact on the protocol will be.