Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
c4-bot-9 opened this issue Feb 29, 2024 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_15_group AI based duplicate group recommendation

Comments

@c4-bot-9
Copy link
Contributor

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-bot-9 c4-bot-9 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Feb 29, 2024
c4-bot-7 added a commit that referenced this issue Feb 29, 2024
@c4-bot-11 c4-bot-11 added the 🤖_15_group AI based duplicate group recommendation label Mar 1, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 3, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added insufficient quality report This report is not of sufficient quality and removed sufficient quality report This report is of sufficient quality labels Mar 3, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Mar 3, 2024
@OpenCoreCH
Copy link

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

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Mar 8, 2024
@c4-judge
Copy link

c4-judge commented Mar 8, 2024

OpenCoreCH changed the severity to QA (Quality Assurance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_15_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

6 participants