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

[M01] Missing slippage control in stableswap's add_liquidity can lead to losses due to frontrunning. #85

Closed
c4-bot-1 opened this issue Feb 27, 2024 · 2 comments
Labels
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 duplicate-93 🤖_15_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-1
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

Liquidity is added to the stableswap pools by calling the add_liquidity function.

pub fn add_liquidity(
        origin: OriginFor<T>,
        pool_id: T::AssetId,
        assets: Vec<AssetAmount<T::AssetId>>,
    ) -> DispatchResult

The issue is that there is no minimum_amount_out parameter in this function, so the liquidity provider has no control over how many tokens get minted.

In curve-style pools, the amount of shares received for adding liquidity to a pool depends on the price of the pool. So a malicious user can do a large swap and then add liquidity to the pool, causing lesser number of shares to be minted to the liquidity provider. To defend against this attack vector, the curve protocol has a slippage control parameter in its contracts.

def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256) -> uint256:
# ...
assert mint_amount >= min_mint_amount, "Slippage screwed you"

Proof of Concept

A malicious user can frontrun the liquidity addition with a large swap. This would cause the amount of shares to be minted to be skewed, and depending on the price and ratio of deposited assets, the liquidity provider can end up with lesser shares than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a min_mint_amount parameter to ensure the liquidity provider does not miss out on shares due to frontrunning. A similar measure is done on the add_liquidity_shares function, but is missing on this function.

Assessed type

MEV

@c4-bot-1 c4-bot-1 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 27, 2024
c4-bot-1 added a commit that referenced this issue Feb 27, 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 duplicate of #158

@c4-judge
Copy link

c4-judge commented Mar 7, 2024

OpenCoreCH marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 duplicate-93 🤖_15_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants