Omnipool's asset_weight_cap can be bypassed, resulting in an unhealthy state of the pool

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

Omnipool's asset_weight_cap can be bypassed, resulting in an unhealthy state of the pool

Proof of Concept

In Omnipool, asset_weight_cap is a safe-guard in place as liquidity restriction, to ensure that any token cannot make up a greater proportion of the Omnipool than initially defined by the cap. See doc.

The problem is asset_weight_cap is only applied in add_liquidity() flow and can be easily bypassed by users calling remove_liquidity().

(1) Success case: add_liquidity() In add_liquidity(), after calculating the new asset state, hub_reserve_ratio is calculated and used to compare the asset_weight_cap. This ensures if too much asset is added, tx will revert with Error::<T>::AssetWeightCapExceeded.

//HydraDX-node/pallets/omnipool/src/lib.rs
        pub fn add_liquidity(origin: OriginFor<T>, asset: T::AssetId, amount: Balance) -> DispatchResult {
...
            let hub_reserve_ratio = FixedU128::checked_from_rational(
                new_asset_state.hub_reserve,
                T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account())
                    .checked_add(*state_changes.asset.delta_hub_reserve)
                    .ok_or(ArithmeticError::Overflow)?,
            )
            .ok_or(ArithmeticError::DivisionByZero)?;

            ensure!(
|>              hub_reserve_ratio <= new_asset_state.weight_cap(),
                Error::<T>::AssetWeightCapExceeded
            );

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L636-L638)

(2)Bypass case: remove_liquidity() But in remove_liquidity(), a new hub_reserve_ratio is not calculated and is not used to compare with asset_weight_cap. The only check is price barrier which only verifies the before-change ratio, and will not be used to verify after-change.

        pub fn remove_liquidity(
            origin: OriginFor<T>,
            position_id: T::PositionItemId,
            amount: Balance,
        ) -> DispatchResult {
...
                        //@audit there is no checks on the new_asset_state after-change. Any asset_weight_cap is accepted.
|>          let new_asset_state = asset_state
                .clone()
                .delta_update(&state_changes.asset)
                .ok_or(ArithmeticError::Overflow)?;

            // Update position state
            let updated_position = position
                .delta_update(
                    &state_changes.delta_position_reserve,
                    &state_changes.delta_position_shares,
                )
                .ok_or(ArithmeticError::Overflow)?;

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

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L773-L782)

Tools Used

Manual

Recommended Mitigation Steps

Add asset_weight_cap checks in remove liquidity as well.

Assessed type

Invalid Validation

[0xRobocop]

Removing liquidity should decrease the weight. Leaving for sponsor review.

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

This report is not valid.

by removing liquidity, the weight of asset has decreased. It does not matter if decreased below the weight cap or it is still above it.

[OpenCoreCH]

As mentioned, the weight will always decrease on withdrawals. If it was already above the cap before for some reason, adding this check would actually be harmful (in that case, removal of liquidity should be encouraged).

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid