Missing hook call will lead to incorrect oracle results

mediumCode4rena

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.

rust📋
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.

rust📋
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.

rust📋
#[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