Missing DUST check in depositSwapWithdraw() and depositDoubleSwapWithdraw()

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L207 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L222 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L382 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L395

Vulnerability details

Impact

swap() requires that the user deposits the swapAmountIn prior to its call. User can make use of deposit() and withdraw() for this which has constraints on the DUST amount:

    function deposit( IERC20 token, uint256 amount ) external nonReentrant
        {
@--->   require( amount > PoolUtils.DUST, "Deposit amount too small");

        _userDeposits[msg.sender][token] += amount;

        // Transfer the tokens from the sender - only tokens without fees should be whitelsited on the DEX
        token.safeTransferFrom(msg.sender, address(this), amount );

        emit TokenDeposit(msg.sender, token, amount);
        }

    function withdraw( IERC20 token, uint256 amount ) external nonReentrant
        {
        require( _userDeposits[msg.sender][token] >= amount, "Insufficient balance to withdraw specified amount" );
@--->   require( amount > PoolUtils.DUST, "Withdraw amount too small");

        _userDeposits[msg.sender][token] -= amount;

        // Send the token to the user
        token.safeTransfer( msg.sender, amount );

        emit TokenWithdrawal(msg.sender, token, amount);
        }

The protocol also provides two similar functions depositSwapWithdraw() and depositDoubleSwapWithdraw() which include the token transfer calls within them. However, no DUST threshold check is made inside them and hence user can always choose to call these two functions to bypass the checks.

Proof of Concept

The two functions missing the DUST check:

    // Deposit tokenIn, swap to tokenOut and then have tokenOut sent to the sender
    function depositSwapWithdraw(IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
        // Transfer the tokens from the sender - only tokens without fees should be whitelisted on the DEX
        swapTokenIn.safeTransferFrom(msg.sender, address(this), swapAmountIn );

        swapAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenIn, swapTokenOut, swapAmountIn, minAmountOut );

        // Send tokenOut to the user
        swapTokenOut.safeTransfer( msg.sender, swapAmountOut );
        }

    // A convenience method to perform two swaps in one transaction
    function depositDoubleSwapWithdraw( IERC20 swapTokenIn, IERC20 swapTokenMiddle, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
        swapTokenIn.safeTransferFrom(msg.sender, address(this), swapAmountIn );

        uint256 middleAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenIn, swapTokenMiddle, swapAmountIn, 0 );
        swapAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenMiddle, swapTokenOut, middleAmountOut, minAmountOut );

        swapTokenOut.safeTransfer( msg.sender, swapAmountOut );
        }

Tools used

Manual inspection

Recommended Mitigation Steps

    // Deposit tokenIn, swap to tokenOut and then have tokenOut sent to the sender
    function depositSwapWithdraw(IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
+       require( swapAmountIn > PoolUtils.DUST, "Deposit amount too small");
        // Transfer the tokens from the sender - only tokens without fees should be whitelisted on the DEX
        swapTokenIn.safeTransferFrom(msg.sender, address(this), swapAmountIn );

        swapAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenIn, swapTokenOut, swapAmountIn, minAmountOut );

        // Send tokenOut to the user
+       require( swapAmountOut > PoolUtils.DUST, "Withdraw amount too small");
        swapTokenOut.safeTransfer( msg.sender, swapAmountOut );
        }

    // A convenience method to perform two swaps in one transaction
    function depositDoubleSwapWithdraw( IERC20 swapTokenIn, IERC20 swapTokenMiddle, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut, uint256 deadline ) external nonReentrant ensureNotExpired(deadline) returns (uint256 swapAmountOut)
        {
+       require( swapAmountIn > PoolUtils.DUST, "Deposit amount too small");
        swapTokenIn.safeTransferFrom(msg.sender, address(this), swapAmountIn );

        uint256 middleAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenIn, swapTokenMiddle, swapAmountIn, 0 );
        swapAmountOut = _adjustReservesForSwapAndAttemptArbitrage(swapTokenMiddle, swapTokenOut, middleAmountOut, minAmountOut );

+       require( swapAmountOut > PoolUtils.DUST, "Withdraw amount too small");
        swapTokenOut.safeTransfer( msg.sender, swapAmountOut );
        }

Assessed type

Invalid Validation

[Picodes]

I don't think there is any need to be above dust when doing swaps here?

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)