Basin - max10afternoon's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L203 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L352 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L460 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L495

Vulnerability details

Impact

On most of the functions that a user can use to receive tokens, the slippage control is performed before the actual transfer of the output tokens, which means that any fee applied on that transaction is ignored, and the user can receive less than what they specified as the minimum output (eg: If the user specifies a minimum of 100 received tokens on a 1% fee on transfer asset. They can end up receiving 99 tokens instead). The impact will vary based on the fee amount and the size of the transfer. On large swap or, more realistically, withdraws it can become significant, but in all cases the user can end up receiving less than specified.

Proof of Concept

The issue effects the swapFromFeeOnTransfer, shift, removeLiquidity and removeLiquidityOneToken functions of the Well contract, in an identical way:

swapFromFeeOnTransfer(https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L203C14-L203C35): Which internally calls _swapFrom (https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L215) to perform the actual transaction (returns amountOut):

        amountOut = reserveJBefore - reserves[j];
        if (amountOut < minAmountOut) {
            revert SlippageOut(amountOut, minAmountOut);
        }

        toToken.safeTransfer(recipient, amountOut);
        emit Swap(fromToken, toToken, amountIn, amountOut, recipient);

As can be seen at the end of the function the slippage check is performed before the transfer of the output tokens, and both the check and return/emit are performed using the input of the transfer function and not on the actual amount that is being transferred to the user.

shift(https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L352):

        if (amountOut >= minAmountOut) {
            tokenOut.safeTransfer(recipient, amountOut);
            reserves[j] -= amountOut;
            _setReserves(_tokens, reserves);
            emit Shift(reserves, tokenOut, amountOut, recipient);
        } else {
            revert SlippageOut(amountOut, minAmountOut);
        }

As can be seen at the end of the function the slippage check is performed before the transfer of the output tokens, and both the check and return/emit are performed using the input of the transfer function and not on the actual amount that is being transferred to the user.

removeLiquidity(https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L460C14-L460C29):

        for (uint256 i; i < _tokens.length; ++i) {
            if (tokenAmountsOut[i] < minTokenAmountsOut[i]) {
                revert SlippageOut(tokenAmountsOut[i], minTokenAmountsOut[i]);
            }
            _tokens[i].safeTransfer(recipient, tokenAmountsOut[i]);
            reserves[i] = reserves[i] - tokenAmountsOut[i];
        }

        _setReserves(_tokens, reserves);
        emit RemoveLiquidity(lpAmountIn, tokenAmountsOut, recipient)

As can be seen at the end of the function the slippage check is performed before the transfer of the output tokens, and both the check and return/emit are performed using the input of the transfer function and not on the actual amount that is being transferred to the user.

removeLiquidityOneToken(https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L495C14-L495C37):

        tokenAmountOut = _getRemoveLiquidityOneTokenOut(lpAmountIn, j, reserves);
        if (tokenAmountOut < minTokenAmountOut) {
            revert SlippageOut(tokenAmountOut, minTokenAmountOut);
        }

        _burn(msg.sender, lpAmountIn);
        tokenOut.safeTransfer(recipient, tokenAmountOut);

        reserves[j] = reserves[j] - tokenAmountOut;
        _setReserves(_tokens, reserves);
        emit RemoveLiquidityOneToken(lpAmountIn, tokenOut, tokenAmountOut, recipient);

As can be seen at the end of the function the slippage check is performed before the transfer of the output tokens, and both the check and return/emit are performed using the input of the transfer function and not on the actual amount that is being transferred to the user.

(Although not meant for swapping fee on transfer tokens, a similar issue can be found in swapTo as well, since the check, to exclude fee on transfer tokens is technically performed on the input tokens and not on the output, by _setReserves. But that would qualify as a misuse by the user).

Here is a foundry script that you can run, since all occurrences of the issues are following the same pattern, the script only exemplify a swap exceeding the minimum amount of tokens received by the user:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import {MockTokenFeeOnTransfer, TestHelper, IERC20, Balances, Call, MockToken, Well} from "test/TestHelper.sol";
import {SwapHelper, SwapAction, Snapshot} from "test/SwapHelper.sol";
import {MockFunctionBad} from "mocks/functions/MockFunctionBad.sol";
import {IWellFunction} from "src/interfaces/IWellFunction.sol";
import {IWell} from "src/interfaces/IWell.sol";
import {IWellErrors} from "src/interfaces/IWellErrors.sol";

import {console} from "forge-std/console.sol";
/**
 * @dev Tests {swapFromFeeOnTransfer} when tokens involved in the swap incur
 * a fee on transfer.
 */
contract WellSwapFromFeeOnTransferFeeTest is SwapHelper {
    function setUp() public {
        tokens = new IERC20[](2);
        tokens[0] = deployMockTokenFeeOnTransfer(0); // token[0] has fee
        tokens[1] = deployMockTokenFeeOnTransfer(1); // token[1] has fee
        setupWell(deployWellFunction(), deployPumps(2), tokens);
        MockTokenFeeOnTransfer(address(tokens[0])).setFee(1e16);
        MockTokenFeeOnTransfer(address(tokens[1])).setFee(1e16);
    }

    /**
     * @dev tokens[0]: 1% fee / tokens[1]: 1% fee
     *
     * Swapping from tokens[0] -> tokens[1]
     */
    function test_swapFromFeeOnTransferWithFee_fromToken() public prank(user) {
        uint256 amountIn = 1e18;
        amountIn = bound(amountIn, 0, tokens[0].balanceOf(address(user)));
        Snapshot memory bef;
        SwapAction memory act;
        
        // Setup delta
        act.i = 0;
        act.j = 1;
        act.userSpends = amountIn;
        act.wellReceives = amountIn - _getFee(act.userSpends);
        act.wellSends = well.getSwapOut(tokens[act.i], tokens[act.j], act.wellReceives);
        act.userReceives = act.wellSends;

        (bef, act) = beforeSwapFrom(act);

        //user's tokens[1] before swap
        uint256 balanceBefore = tokens[act.j].balanceOf(address(user));

        // Perform swap; returns the amount that the Well sent NOT including any transfer fee
        uint256 amountOut = well.swapFromFeeOnTransfer(
            tokens[act.i], tokens[act.j], amountIn, act.userReceives, user, type(uint256).max
        );

        //user's tokens[1] after swap 
        uint256 balanceAfter = tokens[act.j].balanceOf(address(user));

        //actual amount recived
        uint256 actualAmountOut = balanceAfter - balanceBefore;

        console.log("User minimum specified output:");
        console.log(act.userReceives);

        console.log("User has received:");
        console.log(actualAmountOut);

        
        //The amount recived by the user is lesser than the minumum specified
        assertGt(act.userReceives,actualAmountOut );

    }

    function _getFee(uint256 amount) internal view returns (uint256) {
        return amount * MockTokenFeeOnTransfer(address(tokens[0])).fee() / 1e18;
    }
}

Use verbosity options to see the result in the console:

Logs:
  Bound Result 1000000000000000000
  User minimum specified output:
  989020869339354039
  User has received:
  97913066064596049

Recommended Mitigation Steps

Similarly to what the _safeTransferFromFeeOnTransfer function does, the slippage check could be done on a balance after - balance before value.

Assessed type

Token-Transfer

/**
 * @dev Note that `amountOut` is the amount *transferred* by the Well; if a fee
 * is charged on transfers of `toToken`, the amount received by `recipient`
 * will be less than `amountOut`.
 */
function swapFromFeeOnTransfer(
    IERC20 fromToken,
    IERC20 toToken,
    uint256 amountIn,
    uint256 minAmountOut,
    address recipient,
    uint256 deadline
) external nonReentrant expire(deadline) returns (uint256 amountOut) {
    amountIn = _safeTransferFromFeeOnTransfer(fromToken, msg.sender, amountIn);
    amountOut = _swapFrom(fromToken, toToken, amountIn, minAmountOut, recipient);
}