Slippage Attack

From WEB3 Vulnerapedia
Jump to navigation Jump to search

Slippage refers to the price difference between when a market participant submits a DeFi swap trade and the actual price when that trade executes. This difference is usually negligible but can be significant during times of high volatility and for tokens that have low liquidity. Slippage can result in the user receiving more or (often) fewer tokens than they would have otherwise received if the trade had been instantaneous.

DeFi platforms should allow users to specify a slippage parameter "minTokensOut" minimum amount of output tokens to be received from the swap, such that the swap will revert if it wouldn't return the user-specified minimum amount of output tokens. There are several common implementation errors in DeFi systems that developers & auditors should look out for.

No Slippage Parameter

DeFi platforms must allow users to specify a slippage parameter: the minimum amount of tokens they want to be returned from a swap. Auditors should always be on the lookout for swaps which set slippage to 0: 

IUniswapRouterV2(SUSHI_ROUTER).swapExactTokensForTokens(
    toSwap,
    0, // @audit min return 0 tokens; no slippage => user loss of funds
    path,
    address(this),
    now
);

This code tells the swap that the user will accept a minimum amount of 0 output tokens from the swap, opening up the user to a catastrophic loss of funds via MEV bot sandwich attacks. Platforms should also provide a sensible default if the user doesn't specify a value, but user-specified slippage parameters must always override platform defaults. More examples: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32]

No Expiration Deadline

Advanced protocols like Automated Market Makers (AMMs) can allow users to specify a deadline parameter that enforces a time limit by which the transaction must be executed. Without a deadline parameter, the transaction may sit in the mempool and be executed at a much later time potentially resulting in a worse price for the user.

Protocols shouldn't set the deadline to block.timestamp (more on this) as a validator can hold the transaction and the block it is eventually put into will be block.timestamp, so this offers no protection.

Protocols should allow users interacting with AMMs to set expiration deadlines; no expiration deadline may create a potential critical loss of funds vulnerability for any user initiating a swap, especially if there is also no slippage parameter. Examine this high severity finding from Sherlock's BlueBerry Update 1 contest: 

// 2. Swap rewards tokens to debt token
uint256 rewards = _doCutRewardsFee(CRV);
_ensureApprove(CRV, address(swapRouter), rewards);
swapRouter.swapExactTokensForTokens(
    rewards,
    0, // @audit no slippage, can receive 0 output tokens
    swapPath,
    address(this),
    type(uint256).max // @audit no deadline, transaction can 
    // be executed later at more unfavorable time
);

Here "minTokensOut" is hard-coded to 0 so the swap can potentially return 0 output tokens, and the deadline parameter is hard-coded to the max value of utint256, so the transaction can be held & executed at a much later & more unfavorable time to the user. This combination of No Slippage & No Deadline exposes the user to the potential loss of all their input tokens! More examples: [1, 2, 3, 4, 5, 6]

Incorrect Slippage Calculation

The slippage parameter should be something like "minTokensOut" - the minimum amount of tokens the user will accept for the swap. Anything else is a red flag to watch out for as it will likely constitute an incorrect slippage parameter. Consider this simplified code from OpenZeppelin's Origin Dollar Audit:

function withdraw(address _recipient, address _asset, uint256 _amount
) external onlyVault nonReentrant {
    // ...

    // Calculate how much of the pool token we need to withdraw
    (uint256 contractPTokens, , uint256 totalPTokens) = _getTotalPTokens();

    uint256 poolCoinIndex = _getPoolCoinIndex(_asset);
    // Calculate the max amount of the asset we'd get if we withdrew all the
    // platform tokens
    ICurvePool curvePool = ICurvePool(platformAddress);
    uint256 maxAmount = curvePool.calc_withdraw_one_coin(
        totalPTokens,
        int128(poolCoinIndex)
    );

    // Calculate how many platform tokens we need to withdraw the asset amount
    uint256 withdrawPTokens = totalPTokens.mul(_amount).div(maxAmount);

    // Calculate a minimum withdrawal amount
    uint256 assetDecimals = Helpers.getDecimals(_asset);
    // 3crv is 1e18, subtract slippage percentage and scale to asset
    // decimals
    // @audit not using user-provided _amount, but calculating a non-sensical
    // value based on the LP tokens
    uint256 minWithdrawAmount = withdrawPTokens
        .mulTruncate(uint256(1e18).sub(maxSlippage))
        .scaleBy(int8(assetDecimals - 18));

    curvePool.remove_liquidity_one_coin(
        withdrawPTokens,
        int128(poolCoinIndex),
        minWithdrawAmount
    );

    // ...
}

And the fixed version after the audit:

curvePool.remove_liquidity_one_coin(
    withdrawPTokens,
    int128(poolCoinIndex),
    _amount
);

Every smart contract auditor should keep an eye out for any out-of-the-ordinary modifications that protocols make to user-specified slippage parameters. More examples: [1, 2, 3, 4, 5, 6, 7, 8, 9]

Mismatched Slippage Precision

Some platforms allow a user to redeem or withdraw from a set of output tokens with a wide range of different precision values. These platforms must ensure that the slippage parameter "minTokensOut" is scaled to match the precision of the selected output token, else the slippage parameter may be ineffective and lead to precision loss errors. Consider this code from Sherlock's RageTrade contest:

function _convertToToken(address token, address receiver) internal returns (uint256 amountOut) {
    // this value should be whatever glp is received by calling withdraw/redeem to junior vault
    uint256 outputGlp = fsGlp.balanceOf(address(this));

    // using min price of glp because giving in glp
    uint256 glpPrice = _getGlpPrice(false);

    // using max price of token because taking token out of gmx
    uint256 tokenPrice = gmxVault.getMaxPrice(token);

    // @audit always returns 6 decimals, won't work for many tokens
    // apply slippage threshold on top of estimated output amount
    uint256 minTokenOut = outputGlp.mulDiv(glpPrice * (MAX_BPS - slippageThreshold), tokenPrice * MAX_BPS);
    // @audit need to adjust slippage precision to match output
    // token decimals like so:
    // minTokenOut = minTokenOut * 10 ** (token.decimals() - 6);

    // will revert if atleast minTokenOut is not received
    amountOut = rewardRouter.unstakeAndRedeemGlp(address(token), outputGlp, minTokenOut, receiver);
}

"minTokenOut" always returns 6 decimals but the user can specify an output token from a set with a wide range of different precisions, hence the slippage must be scaled to match the output token's precision. More examples: [1]

Minting Exposes Users To Unlimited Slippage

Many DeFi protocols allow users to transfer foreign tokens to mint the protocol's native token - this is functionally the same as a swap where users are swapping foreign tokens for the protocol's native token. Since this is packaged and presented as a "mint", a slippage parameter may be omitted exposing the user to unlimited slippage! Consider this code from Code4rena's Vader audit:

function mintSynth(IERC20 foreignAsset, uint256 nativeDeposit,
                   address from, address to) returns (uint256 amountSynth) {
    // @audit transfers in foreign token
    nativeAsset.safeTransferFrom(from, address(this), nativeDeposit);

    ISynth synth = synthFactory.synths(foreignAsset);

    if (synth == ISynth(_ZERO_ADDRESS))
        synth = synthFactory.createSynth(
            IERC20Extended(address(foreignAsset))
        );

    // @audit frontrunner could manipulate these reserves to influence
    // amount of minted tokens
    (uint112 reserveNative, uint112 reserveForeign, ) = getReserves(
        foreignAsset
    ); // gas savings

    // @audit mints protocol's tokens based upon above reserves
    // this is effectively a swap without allowing the user to
    // specify a slippage parameter, exposing user to unlimited slippage
    amountSynth = VaderMath.calculateSwap(
        nativeDeposit,
        reserveNative,
        reserveForeign
    );

    _update(
        foreignAsset,
        reserveNative + nativeDeposit,
        reserveForeign,
        reserveNative,
        reserveForeign
    );

    synth.mint(to, amountSynth);
}

When implementing minting functions based upon pool reserves or other on-chain data that can be manipulated in real-time, developers should provide & auditors should verify that users can specify slippage parameters, as such mints are effectively swaps by another name! More examples: [1, 3]

MinTokensOut For Intermediate, Not Final Amount

Due to the composable nature of DeFi, a swap can execute multiple operations before returning the final amount of tokens to the user. If the "minTokensOut" parameter is used for an intermediate operation but not to check the final amount, this can result in a loss of funds vulnerability for the user since they may receive fewer tokens than specified. Consider this simplified code from Sherlock's Olympus Update contest:

function withdraw(
    uint256            lpAmount_,
    uint256[] calldata minTokenAmounts_, // @audit slippage param
    bool               claim_
) external override onlyWhileActive onlyOwner nonReentrant returns (uint256, uint256) {
    // ...

    // @audit minTokenAmounts_ enforced here, but this is only
    // an intermediate operation not the final amount received by the user
    // Exit Balancer pool
    _exitBalancerPool(lpAmount_, minTokenAmounts_);

    // Calculate OHM and wstETH amounts received
    uint256 ohmAmountOut = ohm.balanceOf(address(this)) - ohmBefore;
    uint256 wstethAmountOut = wsteth.balanceOf(address(this)) - wstethBefore;

    // Calculate oracle expected wstETH received amount
    // getTknOhmPrice returns the amount of wstETH per 1 OHM based on the oracle price
    uint256 wstethOhmPrice = manager.getTknOhmPrice();
    uint256 expectedWstethAmountOut = (ohmAmountOut * wstethOhmPrice) / _OHM_DECIMALS;

    // @audit this is the final operation but minTokenAmounts_ is no longer
    // enforced, so the amount returned to the user may be less than the
    // minTokenAmounts_ specified, resulting in a loss of funds for the user
    //
    // Take any arbs relative to the oracle price for the Treasury and return the rest to the owner
    uint256 wstethToReturn = wstethAmountOut > expectedWstethAmountOut
        ? expectedWstethAmountOut
        : wstethAmountOut;
    if (wstethAmountOut > wstethToReturn)
        wsteth.safeTransfer(TRSRY(), wstethAmountOut - wstethToReturn);

    // ...
}

Here the user-specified slippage parameter "minTokenAmounts_" is only enforced for the intermediate operation _exitBalancerPool(), after which the output amount of tokens can be further reduced by the treasury skimming the difference between the balancer & oracle expected return amount. Developers & auditors should test & verify that the user-specified "minTokensOut" is always enforced at the final step of a swap before returning the tokens to the user. More examples: [1, 2]

On-Chain Slippage Calculation Can Be Manipulated

Examine this code from Sherlock's Derby contest:

function swapTokensMulti(
    SwapInOut memory                 _swap,
    IController.UniswapParams memory _uniswap,
    bool                             _rewardSwap
    ) public returns (uint256) {
    IERC20(_swap.tokenIn).safeIncreaseAllowance(_uniswap.router, _swap.amount);

    // @audit on-chain slippage calculation can be manipulated,
    // Quoter.quoteExactInput() itself does a swap!
    // https://github.com/Uniswap/v3-periphery/blob/main/contracts/lens/QuoterV2.sol#L138-L146
    // amountOutMinimum must be specified by user, calculated off-chain
    uint256 amountOutMinimum = IQuoter(_uniswap.quoter).quoteExactInput(
      abi.encodePacked(_swap.tokenIn, _uniswap.poolFee, WETH, _uniswap.poolFee, _swap.tokenOut),
      _swap.amount
    );

    uint256 balanceBefore = IERC20(_swap.tokenOut).balanceOf(address(this));
    if (_rewardSwap && balanceBefore > amountOutMinimum) return amountOutMinimum;

    ISwapRouter.ExactInputParams memory params = ISwapRouter.ExactInputParams({
      path: abi.encodePacked(
        _swap.tokenIn,
        _uniswap.poolFee,
        WETH,
        _uniswap.poolFee,
        _swap.tokenOut
      ),
      recipient: address(this),
      deadline: block.timestamp,
      amountIn: _swap.amount,
      amountOutMinimum: amountOutMinimum
    });

    ISwapRouter(_uniswap.router).exactInput(params);
    uint256 balanceAfter = IERC20(_swap.tokenOut).balanceOf(address(this));

    return balanceAfter - balanceBefore;
}

This code attempts to perform on-chain slippage calculation by using Quoter.quoteExactInput() which itself performs a swap and hence is subject to manipulation via sandwich attacks. Developers should ensure & auditors must verify that users are allowed to specify their own slippage parameters which were calculated off-chain. More examples: [1, 2]

Hard-coded Slippage May Freeze User Funds

The idea of setting slippage is to protect the user from getting less tokens than they wanted due to high volatility and stop them from being exploited by MEV bots. So why don't projects just hard-code low slippage to protect users? Because hard-coded slippage can freeze user funds during periods of high volatility. Examine this code from Code4rena's Sturdy contest:

function withdrawCollateral(
    address _asset,
    uint256 _amount,
    address _to
) external virtual {
    // Before withdraw from lending pool, get the stAsset address and withdrawal amount
    // Ex: In Lido vault, it will return stETH address and same amount
    (address _stAsset, uint256 _stAssetAmount) = _getWithdrawalAmount(_asset, _amount);

    // withdraw from lendingPool, it will convert user's aToken to stAsset
    uint256 _amountToWithdraw = ILendingPool(_addressesProvider.getLendingPool()).withdrawFrom(
        _stAsset,
        _stAssetAmount,
        msg.sender,
        address(this)
    );

    // Withdraw from vault, it will convert stAsset to asset and send to user
    // Ex: In Lido vault, it will return ETH or stETH to user
    uint256 withdrawAmount = _withdrawFromYieldPool(_asset, _amountToWithdraw, _to);

    if (_amount == type(uint256).max) {
        uint256 decimal = IERC20Detailed(_asset).decimals();
        _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
    }
    // @audit hard-coded slippage can cause all withdrawals to revert during
    // times of high volatility, freezing user funds. Users should have the option to
    // withdraw during high volatility by setting their own slippage.
    require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

    emit WithdrawCollateral(_asset, _to, _amount);
}

This code sets a very small slippage on withdrawals. While this may protect users from losing funds due to slippage, during times of high volatility when slippage is unavoidable it will also cause all withdrawals to revert, freezing user funds. If a project uses a default slippage, users should always be able to override it with their own slippage to ensure they can transact even during times of high volatility. More examples: [1, 2]

Zero Slippage Required

A function that requires zero slippage is likely to revert presenting a persistent Denial Of Service to users. Expecting zero slippage is unrealistic which is why developers must allow users to specify slippage parameters.

Sources

Article originally researched and written by Dacian

Article: https://dacian.me/defi-slippage-attacks

Retrieved from "https://wiki.r.security/w/index.php?title=Slippage_Attack&oldid=846"