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

1 stars 0 forks source link

[H04] Inefficient Liquidity removal form stableswap can lead to losses and MEV opportunities #142

Open c4-bot-9 opened 6 months ago

c4-bot-9 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L638-L699 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L551-L620

Vulnerability details

Impact

The stableswap pool only allows users to take out liquidity in a single token, via the remove_liquidity_one_asset and withdraw_asset_amount functions. The issue is that when single sided liquidity is removed from a curve pool, it is equivalent to removing liquidity in multiple tokens, and then using the pool to swap all those tokens to the single tokens desired.

Since there is a theoretical swap involved which can incur high slippage, the user can lose value. So when a user makes a large deposit in some tokens, and then decide to withdraw from the pool, they are forced to do so in a single token. So all the liquidity they do is run through a swap and leads to slippage. This means users will always receive less than they put in terms of usd amounts. Furthermore, the token they withdraw in will be overvalued in the pool, and allow MEV searchers to arbitrage down the price.

Basically the protocol forces users to eat slippage, and the money they leave on the table is taken by MEV searchers.

This is different from lack of slippage checks, since the protocol forces the user to take slippage, and adding more checks wont bypass it. Since this is an inefficient design leading to loss by design, it is a critical issue.

Proof of Concept

The bug is based on the fact that adding single sided liquidity to curve manipulates the price. Thus a POC is developed which does the following steps:

Call get_dy on the curve pool to get the expected amount of WETH out given 1 million USDT in Add single sided liquidity with 10_000 WETH Call get_dy again to show the current expected swap results The POC has been developed on Tenderly, using their Forking functionality. Screenshots are linked below in gists.

The relevant screenshots can be found here.

In picture1, we call get_dy with 1 million USDT tokens to swap and get the output in WETH. The result comes out to 528359203036074263596, meaning 1 million USDT can buy 528.3 ETH from the pool.

In picture 2, a single sided deposit is done of 10_000 WETH. 13513336778809589856502 LP tokens are minted to the user. This action drops the price of WETH in the pool.

In picture 3, we call get_dy again wih the same 1 million USDT. Now the result is 819631778518719623368. Thus after the deposit, 819 ETH can be bought with the same USDT amount.

Thus we have established that depositing to the pool decreases the price of WETH, and similarly exiting the pool increases the price of WETH. This price change can be sandwiched by attackers for profit.

The reverse is true when removing liquidity. The price of the token being removed increases and slippage is encountered. A similar MEV opportunity is created, but in the other direction. While the POC above shows the case for adding liquidity, the same is true for removing liquidity.

So users will add liquidity worth 100_000 USD, but when they remove it, it will be less than 100_000 usd even if we ignore any fees. This is an inheren design flaw in the system, and requires no frontrunning or any MEV attacks.

Tools Used

Tenderly

Recommended Mitigation Steps

Allow users to take out liquidity in multiple tokens.

Assessed type

Math

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as primary issue

OpenCoreCH commented 6 months ago

This is by design, the proposed approach would be a different protocol. Downgrading to QA

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)