Incorrect check for DUST threshold in withdraw()

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

deposit()#L207 has the following code snippet to ensure that the balance after deposit remains above DUST:

    // Allow users to deposit tokens into the contract.
    // This is not rewarded or considered staking in any way.  It's simply a way to reduce gas costs by preventing transfers at swap time.
    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);
        }

The require statement in the above code snippet makes perfect sense. The protocol then proceeds to apply a similar check inside the withdraw() function:
withdraw()#L222:

    // Withdraw tokens that were previously deposited
    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 require statement here however makes little sense, and does little to protect against the balance dipping below DUST after a withdraw action. Consider this:

• Let's consider value of DUST = 100
• Alice calls deposit() with amount = 105
• She then calls withdraw() with amount = 101
• Balance of token left is now 105 - 101 = 4 which is less than the DUST amount.

The require condition needs to be amended as per the suggestion made in the section Recommended Mitigation Steps below.

Proof of Concept

Add the following inside src/pools/tests/Pools.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_DUSTafterWithdraw to see it pass:

    function test_DUSTafterWithdraw() public
        {
        address anyone = address(0x1111);
        vm.startPrank( anyone );
        IERC20 someToken = new TestERC20("SOME", 18);
        someToken.approve( address(pools), type(uint256).max );

        pools.deposit(someToken, 105);
        pools.withdraw(someToken, 101);

        assertLt(someToken.balanceOf(address(pools)), PoolUtils.DUST, "pool balance > DUST");
        assertLt(pools.depositedUserBalance(anyone, someToken), PoolUtils.DUST, "user balance > DUST");
        }

Tools used

Foundry

Recommended Mitigation Steps

Amend the require check inside withdraw(). This will allow withdrawal of either the full amount or an amount which keeps the balance above DUST threshold:

    // Withdraw tokens that were previously deposited
    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;
+       require( _userDeposits[msg.sender][token] == 0 || _userDeposits[msg.sender][token] > PoolUtils.DUST, "Withdrawal leaves behind dust amount");

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

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

Assessed type

Invalid Validation

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to Low as the assumption here is that dust is a tiny amount in USD terms