Missing hook call will lead to incorrect oracle results

[c4-bot-1]

Lines of code

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

Vulnerability details

Bug Description

The HydraDx protocol includes an oracle. This oracle generates prices, based upon the information it receives from its sources (of which Omnipool is one). The Omnipool provides information to the oracle through the on_liquidity_changed and on_trade hooks. Whenever a trade happens or the liquidity in one of the pools changes the corresponding hooks need to be called with the updated values.

The Omnipool contract also includes the remove_token() function. This function can only be called by the authority and can be only called on an asset which is FROZEN and where all the liquidity shares are owned by the protocol.

ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
ensure!(
    asset_state.shares == asset_state.protocol_shares,
    Error::<T>::SharesRemaining
);

When the function gets called it transfers all remaining liquidity to the beneficiary and removes the token. This is a change in liquidity in the Omnipool. The functionality in terms of liquidity change is similar to the withdraw_protocol_liquidity() where the protocol also withdraws liquidity in the form of protocol_shares from the pool. When looking at the withdraw_protocol_liquidity() function, one can see that it calls the on_liquidity_changed hook at the end, so that the oracle receives the information about the liquidity change.

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

Unfortunately, the remove_token() function does not call this hook, keeping the oracle in an outdated state. As the token is removed later on, the oracle will calculate based on liquidity that does not exist anymore in the Omnipool.

Impact

The issue results in the oracle receiving incorrect information and calculating new prices, based on an outdated state of the Omnipool.

Proof of Concept

The issue can be viewed when looking at the code of remove_token() where one can see that no call to the hook happens.

#[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 are owned by LPs and the 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(())
}

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by forwarding the updated asset state to the oracle by calling the on_liquidity_changed hook.

Assessed type

Oracle

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #141

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid

[J4X-98]

Hi @OpenCoreCH ,

thank you very much for reviewing this contest. This issue has been deemed as invalid due to a comment by the sponsor on the primary issue #141. #141 describes that in the functions sacrifice_position() and remove_token() a hook call to on_liquidity_changed is missing. The sponsor has disputed this with the claim that in none of those functions, the liquidity gets changed, which is true for sacrifice_position() but not for remove_token(). In sacrifice_position(), the sacrificed positions' ownership is transferred to the protocol but the liquidity does not change.

The same is not the case for the remove_token() function. As one can see in the following code snippet, the function transfers out all liquidity that is owned by protocol shares to a beneficiary, changing the liquidity in the pool.

T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;

The function documentation also mentions the liquidity change.

So contrary to the comment of the sponsor, not only does the token get removed but also the liquidity changes, as the protocol-owned liquidity is sent to the beneficiary. This should result in a call to the hook so that the circuit breaker and the oracle get accordingly updated (and trigger at the right values). This could for example lead to an issue if we have a maximum liquidity change per block of 100 tokens chosen in our circuit breaker and a token gets removed with 90 tokens of protocol liquidity being withdrawn. A later call withdrawing 20 liquidity would incorrectly pass as the earlier withdrawn liquidity is not accounted for due to the missing hook call. This would undermine the security measure of the circuit breaker as the limits are not correctly enforced. Additionally, due to the missing liquidity update, the oracle will be outdated too.

I would like to mention that my issue is the only issue that fully and correctly documents the problem, as #141 is reporting an invalid additional issue and also recommends an incorrect mitigation of increasing the liquidityInBlock in sacrifice_position().

[OpenCoreCH]

Thanks for your comment. After looking at it again, remove_token indeed changes the liquidity like add_token does. While add_token calls on_liquidity_changed, remove_token does not, which can lead to inconsistencies.

[c4-judge]

OpenCoreCH removed the grade

[c4-judge]

OpenCoreCH marked the issue as not a duplicate

[c4-judge]

OpenCoreCH marked the issue as primary issue

[c4-judge]

OpenCoreCH marked the issue as selected for report