Incorrect DUST check for reserve1 when removing liquidity allows attacker to take reserve1 below DUST amount

[c4-bot-9]

Lines of code

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

Vulnerability details

Description

The Pools::removeLiquidity() function removes liquidity for a user by updating the reserves. After the reserves are updated, it performs the following check to ensure that reserve0 and reserve1 are above PoolUtils.DUST.

require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

However, the above check has a typo, and only checks that reserve0 is above PoolUtils.DUST. Due to this, the reserve1 can be made to go below the DUST amount.

Impact

Due to the absence of a validation check, the balance of the reserve1 can fall below the specified DUST threshold. This can potentially disrupt the proper functioning of the following functions, as well as any other external functions that depend on it, as they require the reserves to remain above the DUST threshold:

• Pools::swap()
• Pools::depositSwapWithdraw()
• Pools::depositDoubleSwapWithdraw()

Proof of concept

The test below can be placed in src/pools/tests/Pools.t.sol, and run using the command COVERAGE="yes" NETWORK="sep" forge test -vvv --rpc-url <RPC> --mt "test_poc_takeReserve1BelowDust"

    function test_poc_takeReserve1BelowDust() public {
        vm.startPrank(address(collateralAndLiquidity));

        (uint256 reserves0_1, uint256 reserves1_1) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_1, 1000 ether);
        assertEq(reserves1_1, 1000 ether);

        pools.deposit(tokens[6], 2000 ether);

        // Swap to change the ratio of tokens so that reserve1 has less amount
        pools.swap(
            tokens[6],
            tokens[5],
            1000 ether,
            1 ether,
            block.timestamp + 60
        );

        (uint256 reserves0_2, uint256 reserves1_2) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_2, 2000 ether);
        assertEq(reserves1_2, 500 ether);

        uint256 totalShares = collateralAndLiquidity.totalShares(
            PoolUtils._poolID(tokens[5], tokens[6])
        );

        // Remove enough liquidity to make reserve1 below DUST
        pools.removeLiquidity(
            tokens[6],
            tokens[5],
            ((reserves1_2 - (PoolUtils.DUST - 1)) * (reserves0_2/reserves1_2)), // ((500 ether - 99) * 4),
            1 ether,
            1 ether,
            totalShares
        );

        (uint256 reserves0_3, uint256 reserves1_3) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_3, 396);
        assertLt(reserves1_3, PoolUtils.DUST);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Update the following check to ensure both reserve0 and reserve1 are being checked for the DUST amount.

//-----------------------------------------------------------------V
-require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
+require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #647

[c4-judge]

Picodes marked the issue as satisfactory