The `on_liquidity_changed` hook is not called in some functions

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/traits.rs#L49 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L869-L910 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L1527-L1575

Vulnerability details

Impact

As we can read from the protocol documentation, the calling of on_liquidity_changed hook after a liquidity change is very important. This is because the hook is used to update the oracle and in the circuit breaker. Systems that rely on on_liquidity_changed hooks to calculate or report on liquidity metrics may show inaccurate data since not all liquidity changes are being captured. Also hacker can exploit the absence of liquidity change tracking to manipulate market conditions.

Proof of Concept

on_liquidity_changed hook is used in Omnipool and is very important function that is called when liquidity is added or removed from the pool. It is very important to call it in certain operations because update on-chain oracle and the circuit breaker.

This hook is missing in the sacrifice_position() and remove_token() functions. remove_token() Removes token from Omnipool:

        #[pallet::call_index(12)]
        #[pallet::weight(<T as Config>::WeightInfo::remove_token())]
        #[transactional]
        pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
            T::AuthorityOrigin::ensure_origin(origin)?;
            let asset_state = Self::load_asset_state(asset_id)?;

            // Allow only if no shares owned by LPs and asset is frozen.
            ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
            ensure!(
                asset_state.shares == asset_state.protocol_shares,
                Error::<T>::SharesRemaining
            );
            // Imbalance update
            let imbalance = <HubAssetImbalance<T>>::get();
            let hub_asset_liquidity = Self::get_hub_asset_balance_of_protocol_account();
            let delta_imbalance = hydra_dx_math::omnipool::calculate_delta_imbalance(
                asset_state.hub_reserve,
                I129 {
                    value: imbalance.value,
                    negative: imbalance.negative,
                },
                hub_asset_liquidity,
            )
            .ok_or(ArithmeticError::Overflow)?;
            Self::update_imbalance(BalanceUpdate::Increase(delta_imbalance))?;

            T::Currency::withdraw(T::HubAssetId::get(), &Self::protocol_account(), asset_state.hub_reserve)?;
            T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;
            <Assets<T>>::remove(asset_id);
            Self::deposit_event(Event::TokenRemoved {
                asset_id,
                amount: asset_state.reserve,
                hub_withdrawn: asset_state.hub_reserve,
            });
            Ok(())
        }
    }

sacrifice_position() destroys a position and position's shares become protocol's shares:

        #[pallet::call_index(4)]
        #[pallet::weight(<T as Config>::WeightInfo::sacrifice_position())]
        #[transactional]
        pub fn sacrifice_position(origin: OriginFor<T>, position_id: T::PositionItemId) -> DispatchResult {
            let who = ensure_signed(origin)?;

            let position = Positions::<T>::get(position_id).ok_or(Error::<T>::PositionNotFound)?;

            ensure!(
                T::NFTHandler::owner(&T::NFTCollectionId::get(), &position_id) == Some(who.clone()),
                Error::<T>::Forbidden
            );

            Assets::<T>::try_mutate(position.asset_id, |maybe_asset| -> DispatchResult {
                let asset_state = maybe_asset.as_mut().ok_or(Error::<T>::AssetNotFound)?;

                asset_state.protocol_shares = asset_state
                    .protocol_shares
                    .checked_add(position.shares)
                    .ok_or(ArithmeticError::Overflow)?;

                Ok(())
            })?;

            // Destroy position and burn NFT
            <Positions<T>>::remove(position_id);
            T::NFTHandler::burn(&T::NFTCollectionId::get(), &position_id, Some(&who))?;

            Self::deposit_event(Event::PositionDestroyed {
                position_id,
                owner: who,
            });

            Ok(())
        }

It is very important to call on_liquidity_changed on these operations because it is used to update the oracle and also in the circuit breaker.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Call the on_liquidity_changed hook at the end of these functions.

Assessed type

Other

[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]

The calls is not needed in mentioned functions. sacrifice position does not change any liquidity. and remove-token just removes token .

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)

[c4-judge]

OpenCoreCH marked the issue as duplicate of #51

[c4-judge]

OpenCoreCH marked the issue as partial-50