Users can prevent themselves from getting liquidated indefinitely

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L140-L188
https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L104-L111

Vulnerability details

Impact

The logic for liquidating a user involves calling _decreaseUserShare for the user being liquidated, as their entire WBTC-WETH collateral is being taken. However, in this call, the parameter useCooldown=true, which means that you must wait until cooldownExpiration is reached for a user. A malicious user who is getting liquidated can then abuse this by front-running all calls to liquidate themselves by adding a very small amount of LP such that cooldownExpiration is incremented, making the liquidation call revert. This action can be done indefinitely to prevent liquidations forever at very low cost.

Proof of Concept

When a user is getting liquidated, the 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] );

    // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
@>    _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
    
    ...
    }

As can be seen, there is a call to _decreaseShare with the parameter useCooldown set to true. Let's see how this is used in the StakingRewards:_decreaseUserShare function call:

if ( useCooldown )
if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
    {
@>    require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

    // Update the cooldown expiration for future transactions
    user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
    }

As can be seen, (block.timestamp >= user.cooldownExpiration) is required, else this call will fail. Although this is already an issue in itself, a malicious user can abuse this to continuously increase user.cooldownExpiration every time there is an attempt to liquidate them by simply calling CollateralAndLiquidity:depositCollateralAndIncreaseShare with maxAmountWBTC=101, maxAmountWETH=101. This function does in internal call to _depositLiquidityAndIncreaseShare, which includes the following logic:

_increaseUserShare( msg.sender, poolID, addedLiquidity, true );

Here useCooldown is also true, meaning that user.cooldownExpiration is updated, allowing the attacker to prevent themselves from getting liquidated.

Tools Used

Manual review

Recommended Mitigation Steps

In CollateralAndLiquidity:liquidateUser on line 154, the call to _decreaseUserShare should not have useCooldown=true.

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked issue #312 as primary and marked this issue as a duplicate of 312

[c4-judge]

Picodes marked the issue as satisfactory