[H01] Share manipulation attack due to wrong check in withdraw_asset_amount function

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

The stableswap pool allows users to send in any tokens to the pool, as confirmed by the dev on the discord channel. This opens it up to a potential attack vector, where initial user can deposit a small amount of tokens, and then "donate" a large amount of tokens such that the share ratio is set really high. This will lead to rounding losses and potentially block other users from making small sized deposits. The protocol prevents this attack vector by instituting a T::MinPoolLiquidity check, ensuring that the amount of LP shares minted is always either 0 or above this minimum threshold.

This is reflected in the liquidity addition functions, where the minimum amount check is enforced.

ensure!(
        asset.amount >= T::MinTradingLimit::get(),
        Error::<T>::InsufficientTradingAmount
    );

This is also implemented in the function remove_liquidity_one_asset.

let share_issuance = T::Currency::total_issuance(pool_id);

    ensure!(
        share_issuance == share_amount
            || share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
        Error::<T>::InsufficientLiquidityRemaining
    );

Where share_issuance checks the total supply of the pool token. However, the check in withdraw_asset_amount is faulty.

let current_share_balance = T::Currency::free_balance(pool_id, &who);
    ensure!(
        current_share_balance == shares
            || current_share_balance.saturating_sub(shares) >= T::MinPoolLiquidity::get(),
        Error::<T>::InsufficientShareBalance
    );

Instead of checking against the total issuance, here the contract checks against the user's balance. Thus, users are allowed to redeem all the LP tokens in their account, even if that leads to the situation where the total issuance is below the threshold, allowing an inflation attack. Users can just send 1 wei of LP token to some other account, and then the current_share_balance == shares will pass when redeeming the remaining balance, allowing LP removal and leaving 1 wei of LP still existing.

Since inflation attacks and DOS attacks can be carried out due to this valid pool state, this is a high severity issue. The recent Wise lending hack was also due to such inflation attacks.

Proof of Concept

The user can trigger this situation by following these steps.

1. Alice deposits liquidity and gets 1*ONE LP tokens.
2. Alice sends 1 wei of that LP otken to BOB. ALice now has 1*ONE-1 LP tokens.
3. Alice calls withdraw_asset_amount function with all her LP token balance. Since current_share_balance == shares check passes, all LP tokens are redeemed, except for the 1 wei in BOB's account
4. Pool now has only 1 wei of LP tokens issued. Users can "donate" tokens to the pool to raise the share ratio.

Tools Used

Manual Review

Recommended Mitigation Steps

Check with share_issuance instead of the user's balance, just like in remove_liquidity_one_asset function.

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #154

[OpenCoreCH]

Does not demonstrate worse impact than #154, grouping and medium is appropriate here.

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)