Ema-oracle will show an arbitrary asset price, even though the asset was completely removed from the omnipool

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1567-L1573

Vulnerability details

Impact

Ema-oracle will show an arbitrary asset price, even though the asset was completely removed from the omnipool. Any other flows that consume ema-oracle price can be impacted as well.

Proof of Concept

The vulnerability is in the omnipool pallet (lib.rs) remove_token(). Typically, any liquidity management/trading will invoke a hook that calls ema-oracle pallet to update a spot oracle entry.

However, the exception is that no hook is called in remove_token(), even though remove_token() also changes liquidity and asset reserves, on top of completely deleting the asset from the pool.

The impact is ema-oracle will still show an oracle entry price with an arbitrary value at user remove liquidity (remove_liquidity), even though the asset reserve is completely withdrawn in remove_token() and the asset no longer exists.

(1)For comparison, this is an example of a normal flow of liquidity change.

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn remove_liquidity(
			origin: OriginFor<T>,
			position_id: T::PositionItemId,
			amount: Balance,
		) -> DispatchResult {
...
                        //@audit-info note: this calls adapter which calls ema-oracle to record an oracle entry
|>			T::OmnipoolHooks::on_liquidity_changed(origin, info)?;

			Ok(())
		}

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L864)

(2) Problem: remove_token() doesn't call any hook to alert ema-oracle pallets the reserve is withdrawn and asset is deleted.

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
...
			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)?;
                        //@audit remove_token() flow will simply withdraw reserve, and share but will not alert ema-oracle with any hook for the liquidity and reserve change.
|>			<Assets<T>>::remove(asset_id);
			Self::deposit_event(Event::TokenRemoved {
				asset_id,
				amount: asset_state.reserve,
				hub_withdrawn: asset_state.hub_reserve,
			});
			Ok(())
		}

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1567-L1573)

Tools Used

Manual

Recommended Mitigation Steps

In remove_token(), add a hook that alerts ema-oracle the asset is withdrawn, and update the ema-oracle entry accordingly.

Assessed type

Error

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #73

[OpenCoreCH]

Unlike #73, this does not demonstrate any relevant negative impact -> downgrading to QA

[c4-judge]

OpenCoreCH marked the issue as not a duplicate

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a