`reserves.reserve1` can become < `PoolUtils.DUST`

[c4-bot-2]

Lines of code

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

Vulnerability details

Summary

Pools.removeLiquidity does not check that reserves.reserve1 >= PoolUtils.DUST at the end.

Vulnerability Details

Pools.removeLiquidity contains the following line: require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal"); It should be reserve1 instead of the second reserve0 after &&.

Impact

One of the invariants is broken, which may lead to different errors.

Proof of Concept

Place this in src/pools/tests/InflationAttack.t.sol and run COVERAGE="yes" forge test -f wss://ethereum-sepolia.publicnode.com -vvv --mc InflationAttack

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../../dev/Deployment.sol";
import "../PoolUtils.sol";

contract InflationAttack is Deployment {
    TestERC20 immutable tokenA;
    TestERC20 immutable tokenB;
    address ALICE = address(0x1111);
    address BOB = address(0x2222);

    function _prepareToken(TestERC20 token) internal {
        vm.startPrank(DEPLOYER);
        token.approve(address(pools), type(uint256).max);
        token.approve(address(collateralAndLiquidity), type(uint256).max);
        token.transfer(ALICE, 1 ether);
        token.transfer(BOB, 1 ether);

        vm.startPrank(ALICE);
        token.approve(address(pools), type(uint256).max);
        token.approve(address(collateralAndLiquidity), type(uint256).max);

        vm.startPrank(BOB);
        token.approve(address(pools), type(uint256).max);
        token.approve(address(collateralAndLiquidity), type(uint256).max);
    }

    constructor() {
        initializeContracts();

        grantAccessAlice();
        grantAccessBob();
        grantAccessCharlie();
        grantAccessDeployer();
        grantAccessDefault();

        finalizeBootstrap();

        vm.startPrank(address(daoVestingWallet));
        salt.transfer(DEPLOYER, 1000000 ether);
        salt.transfer(address(collateralAndLiquidity), 1000000 ether);
        vm.stopPrank();

        vm.startPrank(DEPLOYER);
        tokenA = new TestERC20("TEST", 18);
        tokenB = new TestERC20("TEST", 18);
        vm.stopPrank();
        _prepareToken(tokenA);
        _prepareToken(tokenB);

        vm.stopPrank();
        vm.prank(address(dao));
        poolsConfig.whitelistPool(pools, tokenA, tokenB);
        vm.stopPrank();
    }

    function testAttack() external {
        bytes32 poolId = PoolUtils._poolID(tokenA, tokenB);

        // Ensure the pool is empty
        (uint256 reserve0, uint256 reserve1) = pools.getPoolReserves(tokenA, tokenB);
        assertEq(reserve0, 0);
        assertEq(reserve1, 0);

        uint amountToDeposit0 = PoolUtils.DUST * 101;
        uint amountToDeposit1 = PoolUtils.DUST + 1;

        // Deposit enough to pass the DUST check
        vm.startPrank(ALICE);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare( 
            tokenA, tokenB, 
            amountToDeposit0, amountToDeposit1, // maxAmount
            1, // minLiquidityReceived
            block.timestamp, false 
        );

        // Check the liquidity received
        uint userShareForPool = collateralAndLiquidity.userShareForPool(ALICE, poolId);
        uint totalShares = collateralAndLiquidity.totalShares(poolId);
        assertEq(userShareForPool, amountToDeposit0 + amountToDeposit1);
        assertEq(totalShares, amountToDeposit0 + amountToDeposit1);

        (reserve0, reserve1) = pools.getPoolReserves(tokenA, tokenB);
        assertEq(reserve0, amountToDeposit0);
        assertEq(reserve1, amountToDeposit1);
        console.log(reserve0, reserve1, collateralAndLiquidity.userShareForPool(ALICE, poolId));

        vm.warp(block.timestamp + stakingConfig.modificationCooldown());
        collateralAndLiquidity.withdrawLiquidityAndClaim(
            tokenA, tokenB, 
            userShareForPool * 100 / 101,
            0, 0, type(uint).max
        );
        (reserve0, reserve1) = pools.getPoolReserves(tokenA, tokenB);
        assertEq(reserve0, 100);
        assertEq(reserve1, 1);
        console.log(reserve0, reserve1, collateralAndLiquidity.userShareForPool(ALICE, poolId));
    }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Use reserve1 instead of the second reserve0 after &&. Replace the line with 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