When there is a single WETH-WBTC LP, they cannot be liquidated

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

Irrespective of how many / who is LPing in the WETH-WBTC collateral pool, there should never be a case in which a user cannot be liquidated. However in the case that there is a single LP, the liquidation logic of CollateralAndLiquidity:liquidateUser will revert due to attempting to remove all of the LP tokens for that single LP provider. This in turn will remove the entire reserve balance of both tokens from the pool, which will cause a check that the token reserves >= DUST to revert. Ultimately this means that the user can never get liquidated. This is High severity and Low probability, therefore Medium.

Proof of Concept

Assume that there is a single large LP provider for the WETH-WBTC pool, who has minted USDS against their collateral, and they are now undercollateralized and should be liquidatable. To do so, a liquidator will call CollateralAndLiquidity:liquidateUser which is defined as follows:

function liquidateUser( address wallet ) external nonReentrant
   {
   require( wallet != msg.sender, "Cannot liquidate self" );

   // First, make sure that the user's collateral ratio is below the required level
   require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

   uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

   // Withdraw the liquidated collateral from the liquidity pool.
   // The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
@>   (uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );
   ...
   }

As part of this call, there is an attempt to remove all of the user's collateral with pools.removeLiquidity(..). Pools:removeLiquidity is defined as follows:

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.
@>     require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
   ...
   }

Notice that since the LP has the entire pool's liquidity, removing their position means that e.g. reclaimedA=reserves.reserve0. This means that in the update to reserves.reserve0 and reserves.reserve1, they will both be set to 0, and so the require statement will revert. This reversion bubbles up and means that the user can never be liquidated.

Tools Used

Manual review

Recommended Mitigation Steps

The logic for liquidateUser should be updated so that the maximum collateral removed for a user will result in reserves.reserve0>=DUST and reserves.reserve1>=DUST.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #278

[c4-judge]

Picodes marked the issue as satisfactory