search

code-423n4 / 2024-02-hydradx-findings

1 stars 0 forks source link

Using `pallet_balances::Call::transfer_allow_death` to send native token to the omnipool protocol account can manipulate the native token spot price #98

Open c4-bot-2 opened 8 months ago

c4-bot-2 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L597-L603 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L746-L752 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L759-L764

Vulnerability details

Impact

A medium risk finding [A1] Oracle is not updated by directly sending assets to an Omnipool in the previous EMA Oracle audit still exists in the HDX sub-pool, the previous audit outlines the impact of this vulnerability to the EMA Oracle including misleading exchange rate and influence to the honest user's operation, which still apply to the current implementation of the Omnipool.

Moreover, in the add_liquidity and remove_liquidity there is a check to ensure the price difference between the spot price and the oracle price can't be greater than 1% otherwise the add_liquidity/remove_liquidity will fail. The oracle price is hard to manipulate but the attacker can directly send HDX to the protocol account to manipulate the spot price of HDX thus manipulating the price difference and further the result of the check.

When the check is supposed to pass, by manipulating the spot price of HDX to make the check fail, the attacker can prevent any liquidity change in the HDX sub-pool. The LPs of the HDX sub-pool are incentivized to launch such an attack because by preventing other LPs join the HDX sub-pool, they can earn more fees proportionate to the volume of the HDX sub-pool without worrying about other LPs joining and splitting the fee.

When the check is supposed to fail, by manipulating the spot price of HDX to make the check pass, the attacker can break the first mitigation step of a known/mitigated critical vulnerability. Also, the withdrawal fee in the second mitigation step is determined by the price difference between the spot price and the Short period oracle price, so (even without flipping the check result) by manipulating the price difference the attacker can pay less withdrawal fee thus cause loss to the pool.

The cost of the attack is low for a large LP of the HDX sub-pool, because he owns a large proportion of the share of the pool, and a corresponding large proportion of HDX that is directly sent to the protocol account still belongs to him.

Proof of Concept

The CallFilter is intended to fix [A1] Oracle is not updated by directly sending assets to an Omnipool but it doesn't filter the pallet_balances::Call::transfer_allow_death call, which can be utilized by the attacker to directly send HDX to the protocol account. This can be reproduced in polkadot.js by:

  1. In https://polkadot.js.org/apps/#/explorer, select HydraDX and click Fork Locally to create a local fork of HydraDX with Chopsticks
  2. Send HDX from Alice to the Omnipool protocol account (7L53bUT..) with balances.transferAllowDeath
  3. The Omnipool protocol account's HDX free balance is increased

In the Omnipool, the spot price of an asset is a combination of the free balance of the asset that is directly queried from the protocol account and the hub_reserve tracking in the asset state, by sending HDX to the protocol account the spot price of HDX can be manipulated.

In both the add_liquidity and remove_liquidity, the spot price is used directly to perform the price difference check against the EMA Oracle [1][2], and in remove_liquidity the price difference between the spot price and the Short period oracle price is used to calculate the withdrawal fee.

The oracle prices used in the price difference check are the Short period oracle price and the LastBlock period oracle price (the check only passes if the difference with both oracle prices is within the limit), and the limit is set to 1%. The LastBlock period oracle price is used for the withdrawal fee calculation.

Tools Used

Manual inspection

Recommended Mitigation Steps

Add the pallet_balances::Call::transfer_allow_death to the CallFilter to avoid directly sending assets to the protocol account completely.

Assessed type

Invalid Validation

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as insufficient quality report

0xRobocop commented 8 months ago

Is not clear how the attacker will profit by donating tokens to the pool.

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as primary issue

OpenCoreCH commented 8 months ago

@enthusiastmartin This was not shown to you in the sponsor review and I would like to hear your opinion here. Do you consider this to be an issue or is it fine that HDX can be transferred to the protocol account?

enthusiastmartin commented 8 months ago

@enthusiastmartin This was not shown to you in the sponsor review and I would like to hear your opinion here. Do you consider this to be an issue or is it fine that HDX can be transferred to the protocol account?

Although there is no obvious problem when HDX is donated to omnipool, we have all transfers to omnipool account explicitly filtered. So adding this too makes sense, but i disagree with severity. It might be considered low/QA

OpenCoreCH commented 8 months ago

Downgrading to QA because the demonstrated impact here is very small: An attacker would have to actively spend funds to (temporarily) manipulate the withdrawal fee or the price difference checks. While it is possible that the manipulation of these parts have other implications, this was not demonstrated here.

c4-judge commented 8 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)

c4-judge commented 8 months ago

OpenCoreCH marked the issue as grade-a