Some functions lack slippage protection

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L577 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L995 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1118

Vulnerability details

Impact

Some of the functions in omnipool and stableswap lack slippage protection, and users may not expect the same results when they add liquidity.

For example : omnipool#add_liquidity,hub_asset_liquidity is variable, The share amount available to the user is calculated using calculate_add_liquidity_state_changes, which relies on hub_asset_liquidity, so the user may receive less share amount than expected.

Proof of Concept

omnipool and stableswap's user-facing function,buy sell, have slippage protection added, for example: omnipool's buy function can set max_sell_amount.

slippage protection is also added to add_liquidity_shares in the stablepool, and max_asset_amount can be set:

    #[require_transactional]
    fn do_add_liquidity_shares(
        who: &T::AccountId,
        pool_id: T::AssetId,
        shares: Balance,
        asset_id: T::AssetId,
        max_asset_amount: Balance,
    ) -> Result<Balance, DispatchError> {
        .....
        ensure!(amount_in <= max_asset_amount, Error::<T>::SlippageLimit);
    }

The problem is that there are other functions that add liquidity that don't have slippage protection:

1. stableswap#do_add_liquidity do_add_liquidity functions similarly to do_add_liquidity_shares, adding liquidity to stablepool, do_add_liquidity can add liquidity to multiple assets, However, do_add_liquidity does not set SlippageLimit as do_add_liquidity_shares does.

2. omnipool#add_liquidity in omnipool#add_liquidity,hub_asset_liquidity is variable, The share amount available to the user is calculated using calculate_add_liquidity_state_changes, which relies on hub_asset_liquidity, so the user may receive less share amount than expected.

    #[require_transactional]
    fn do_add_liquidity(
        who: &T::AccountId,
        pool_id: T::AssetId,
        assets: &[AssetAmount<T::AssetId>],
    ) -> Result<Balance, DispatchError> {

@>      let current_hub_asset_liquidity =
                T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account());

        let state_changes = hydra_dx_math::omnipool::calculate_add_liquidity_state_changes(
            &(&asset_state).into(),
            amount,
            I129 {
                value: current_imbalance.value,
                negative: current_imbalance.negative,
            },
@>          current_hub_asset_liquidity,
        )
        .ok_or(ArithmeticError::Overflow)?;

        ....
        let lp_position = Position::<Balance, T::AssetId> {
            asset_id: asset,
            amount,
@>          shares: *state_changes.asset.delta_shares,
            // Note: position needs price after asset state is updated.
            price: (new_asset_state.hub_reserve, new_asset_state.reserve),
        };
        .....
    }

3. omnipool#remove_liquidity Similar to add_liquidity

Tools Used

vscode, manual

Recommended Mitigation Steps

Add slippage protection to all necessary functions

Assessed type

MEV

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[0xRobocop]

Omnipool provides slippage protection via PriceBarrier::ensure_price. Leaving for sponsor review.

[c4-sponsor]

enthusiastmartin (sponsor) confirmed

[c4-sponsor]

enthusiastmartin marked the issue as disagree with severity

[OpenCoreCH]

Valid medium because there is a hypothetical path to leak values and in-line with other contests were missing slippage checks were medium.

[c4-judge]

OpenCoreCH marked issue #93 as primary and marked this issue as a duplicate of 93

[c4-judge]

OpenCoreCH marked the issue as satisfactory