c4-bot-7 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

Extrinsic calls are accounted for less fees, an indirect loss for the block producer.

Proof of Concept

In remove_liquidity, we only add the T::OmnipoolHooks::on_liquidity_changed_weight() in the weight calculation. However, we also use T::ExternalPriceOracle::get_price for both safe and unsafe withdraws.

            let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();
            // Skip price check if safe withdrawal - trading disabled.
            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)?;

So T::ExternalPriceOracle::get_price_weight() should be added to the weight calculation. Otherwise, the weight is less than actually used, and attackers can make more indirect losses by calling remove_liquidity many times with low share value.

Tools Used

Manual review.

Recommended Mitigation Steps

2 * T::ExternalPriceOracle::get_price_weight() (if it's a safe withdraw, we will call the oracle twice) should be added for remove_liquidity.

Assessed type

Context

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #53

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)