`remove_liquidity_one_asset()` and `sell()` functions has no slippage protection when using fee-on-transfer tokens

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L602-L606 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L750-L753

Vulnerability details

Impact

The remove_liquidity_one_asset() and sell() functions do not have adequate slippage protection.

Proof of Concept

remove_liquidity_one_asset() function allows a liquidity provider (LP) to remove liquidity from a specific pool in exchange for a selected asset. sell() function execute a swap of asset_in for asset_out. Both functions have slippage protection via the min_amount_out or min_buy_amount variable.

'min_amount_out': minimum amount to receive
...
`min_buy_amount`: Minimum amount required to receive

Problem is that this value is checked before the transfer. For example lets see remove_liquidity_one_asset():

    ensure!(amount >= min_amount_out, Error::<T>::SlippageLimit);

    // Burn shares and transfer asset to user.
    T::Currency::withdraw(pool_id, &who, share_amount)?;
    T::Currency::transfer(asset_id, &pool_account, &who, amount)?;

As we see first we do the check and then the transfer. This means that a user will choose how many tokens they want, but if the token is a fee-on-transfer then min_amount_out will not be adequate and the user will receive fewer tokens than they chose to receive as a minimum.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Track how many tokens the user will receive after the transfer and only then do slippage check.

Assessed type

Token-Transfer

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[OpenCoreCH]

Fee-on-transfer not relevant here, see #175

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid