Insufficient Price Protection in Liquidity Actions May Enable Manipulation

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L577-L607
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1165-L1187

Vulnerability details

Description

There are a few ways an attacker could potentially manipulate prices in the Omnipool to profit from other liquidity providers (LPs), Sandwich attack on liquidity actions. An attacker could execute a sandwich attack around a large liquidity action by another LP.

For example:

1. Attacker swaps asset A for asset B, pushing the price of A up and B down.
2. LP adds/removes liquidity, receiving less favorable pricing due to the attacker's swap.
3. Attacker swaps asset B back for asset A, reversing the price change.
4. Attacker profits from the price arbitrage at the expense of the LP.

This is possible because there are currently no slippage limits on liquidity actions.

The Omnipool pallet allows for the pooled trading of assets, with the on-chain prices used to value liquidity additions and withdrawals. However, the current design does not sufficiently protect these pricing queries from manipulation. An attacker could exploit this by pushing prices before a victim's transaction, draining value in the process.

Impact

In the add_liquidity and remove_liquidity functions in the Omnipool pallet. When liquidity is added or removed, the code calculates the amount of pool shares to mint or burn based on the current on-chain price. However, there are no checks that this price matches external prices. This leaves the door open to manipulation.

scenario

1. Alice decides to add a large amount of liquidity to the Omnipool, e.g. 1,000 DOT.

2. Eve notices this intent and quickly executes a swap of 1 DOT for 1 KSM right before Alice's transaction is included.

3. This swap changes the on-chain DOT/KSM price used to calculate Alice's minted shares. Eve got 1 KSM for cheap due to low liquidity.

4. Alice's transaction goes through. The manipulated price causes her to get less pool shares than she should have.

5. Eve immediately swaps the 1 KSM back for DOT, reversing the price change. She pockets the difference as profit.

Impact

This attack would cause Alice to lose out on pool share value. It also damages integrity of the system. Over time, this may cause users to lose trust in the Omnipool.

Proof of Concept

HydraDX-node/pallets/omnipool/src/lib.rs#fn add_liquidity

HydraDX-node/pallets/stableswap/src/lib.rs#fn calculate_shares

fn add_liquidity(...) {

  // Calculate pool shares
  shares = calculate_shares(reserves, amounts, amplification); 
  
  // Mint shares

}

fn calculate_shares(...) {

  // Uses on-chain price 
  price = get_current_price(reserves);
  
  // Calculates shares based on price  
}

There is no check that price matches an external reference like an oracle. This allows manipulation.

Specifically, in omnipool::add_liquidity, there is a price check using PriceBarrier: HydraDX-node/pallets/omnipool/src/lib.rs#L597-L601

T::PriceBarrier::ensure_price(
  &who,
  T::HubAssetId::get(),
  asset,
  EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
)

However, this PriceBarrier only does a basic check that the on-chain price is within expected bounds. It does not prevent manipulation itself.

Similarly in https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1165-L1187, there is no external price check when calculating shares to mint based on the reserves and amounts added. This could potentially be manipulated.

Step to Reproduce
Preconditions

• Attacker and victim have accounts set up
• Omnipool is deployed with assets DOT and KSM
• Oracle price feed is streaming prices for DOT/KSM
• Victim intends to add/remove large liquidity to the pool

Attack Steps

1. Attacker monitors chain activity and spots victim's intent to add/remove liquidity

2. Right before victim sends liquidity transaction, attacker quickly swaps a small amount of DOT for KSM

   • This
   • swap changes the DOT/KSM price ratio

3. Victim's transaction executes, minting/burning shares based on manipulated price

4. Attacker immediately swaps KSM back for DOT, reversing the price change

   • Attacker pockets the difference as profit

5. Victim's wallet shows less DOT/KSM than intended due to sandwich attack

   • Loss is realized once victim burns shares back to original assets

Tools Used

Manual Review

Recommended Mitigation Steps

• Price slippage checks against an oracle
• Limits on price movements
• Delay period for price changes before big liquidity actions take effect

Solution

The key improvement would be to incorporate a stronger price protection mechanism before large liquidity actions like:

• Delayed pricing from a robust oracle
• Monitoring size of price changes
• Slippage limits enforced

Without stronger measures, the liquidity action pricing remains vulnerable to manipulation attacks via swaps right before, as described in the scenario.

Assessed type

Oracle

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory