`Pools#removeLiquidity()` will be reverted if `liquidityToRemove` is equal to `totalLiquidity`

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The last liquidity provider is blocked to withdraw all their liquidity.

Proof of Concept

Any user can add liquidity into a whitelisted pool by calling Liquidity#depositLiquidityAndIncreaseShare(). The liquidity provider can remove their liquidity at any time by calling Liquidity#withdrawLiquidityAndClaim(). When removing liquidity, A check will be performed to ensure that the reserves of both tokens do not fall below the specified DUST threshold.

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

However, if the last liquidity provider of a whitelisted pool try to remove all their liquidity, it will be blocked due to above inspection. Copy below codes to DAO.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testRemoveAllLiquidity

  function testRemoveAllLiquidity() public
  {
    vm.startPrank(address(collateralAndLiquidity));
    IERC20 token0 = new TestERC20("TEST", 18);
    IERC20 token1 = new TestERC20("TEST", 6);
    vm.stopPrank();

    vm.prank(address(dao));
    poolsConfig.whitelistPool( pools, token0, token1);

    vm.startPrank(address(collateralAndLiquidity));
    token0.approve(address(pools), type(uint256).max);
    token1.approve(address(pools), type(uint256).max);

    uint256 added0 = 2000 ether;
    uint256 added1 = 1000 * 10 ** 6;
    uint256 liquidityAdded;

    (added0, added1, liquidityAdded) = pools.addLiquidity(token0, token1, added0, added1, 0, 0 );
    // Can't remove all the liquidity
    vm.expectRevert( "Insufficient reserves after liquidity removal" );
    pools.removeLiquidity(token0, token1, liquidityAdded, 0, 0, liquidityAdded);
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Even the last liquidity provider should be allowed to withdraw all of their provided liquidity.

  function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
  {
    require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
    require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

    (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

    // Determine how much liquidity is being withdrawn and round down in favor of the protocol
    PoolReserves storage reserves = _poolReserves[poolID];
    reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
    reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

    reserves.reserve0 -= uint128(reclaimedA);
    reserves.reserve1 -= uint128(reclaimedB);

    // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
    // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
+ if (reserves.reserve0 != 0 || reserves.reserve1 != 0)
    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

    // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
    if (flipped)
      (reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);

    require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

    // Send the reclaimed tokens to the user
    tokenA.safeTransfer( msg.sender, reclaimedA );
    tokenB.safeTransfer( msg.sender, reclaimedB );

    emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
  }

By fixing Pools#removeLiquidity(), the last collateral provider can also withdraw all their collateral correctly by calling withdrawCollateralAndClaim().

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #459

[c4-judge]

Picodes marked the issue as satisfactory