c4-bot-1 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

The HDX/LRNA pool is not protected by the MaxInRatio and circuit breaker through update_hdx_subpool_hub_asset, making this pool vulnerable to attacks when the liquidity is low.

Proof of Concept

When users buy/sell assets, we make sure the input assets are not beyond the MaxInRatio, and call the circuit breaker in case of exceeding trading volume.

When users buy/sell assets, there is a protocol fee being charged and added to the HDX/LRNA pool in update_hdx_subpool_hub_asset.

    /// Update Hub asset side of HDX subpool and add given amount to hub_asset_reserve
    fn update_hdx_subpool_hub_asset(origin: T::RuntimeOrigin, hub_asset_amount: Balance) -> DispatchResult {
        if hub_asset_amount > Balance::zero() {
            let hdx_state = Self::load_asset_state(T::HdxAssetId::get())?;

            let mut native_subpool = Assets::<T>::get(T::HdxAssetId::get()).ok_or(Error::<T>::AssetNotFound)?;
            native_subpool.hub_reserve = native_subpool
                .hub_reserve
                .checked_add(hub_asset_amount)
                .ok_or(ArithmeticError::Overflow)?;
            <Assets<T>>::insert(T::HdxAssetId::get(), native_subpool);

            let updated_hdx_state = Self::load_asset_state(T::HdxAssetId::get())?;

            let delta_changes = AssetStateChange {
                delta_hub_reserve: BalanceUpdate::Increase(hub_asset_amount),
                ..Default::default()
            };

            let info: AssetInfo<T::AssetId, Balance> = AssetInfo::new(
                T::HdxAssetId::get(),
                &hdx_state,
                &updated_hdx_state,
                &delta_changes,
                false,
            );

            T::OmnipoolHooks::on_liquidity_changed(origin, info)?;
        }
        Ok(())
    }

As we can see, there is no protection on the input size and trading volume for the HDX pool, so it's vulnerable to attacks when the liquidity is low (the attacker can keep triggering the protocol fee charges to quickly increase the hdx price).

Tools Used

Manual Review.

Recommended Mitigation Steps

In update_hdx_subpool_hub_asset, we should add:

ensure!(
                hub_asset_amount
                    <= hdx_state
                        .hub_reserve
                        .checked_div(T::MaxInRatio::get())
                        .ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxInRatio is zero.
                Error::<T>::MaxInRatioExceeded
            )

And call T::OmnipoolHooks::on_liquidity_changed() for the HDX assets.