User can become un-liquidatable by perpetually extending his cooldown

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

A malicious user can avoid liquidation by perpetually extending his cooldown period which will result in bad debt for the protocol.

Proof of Concept

Let's take a look at the liquidateUser function in CollateralAndLiquidity.sol:

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 );

//unrelated functionality below
        }

We're calling the _decreaseUserShare function in the StakingRewards.sol contract with true passed as useCooldown:

    function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
        {
        require( decreaseShareAmount != 0, "Cannot decrease zero share" );

        UserShareInfo storage user = _userShareInfo[wallet][poolID];
        require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );

        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();
            }
//unrelated functionality below
}

As you can see, we'll enter the 1st if, and given that the msg.sender is a random liquidator, we'll enter the 2nd if as well. Which gets us to this:

    require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

This means the user can't get liquidated if the cooldown hasn't expired. The default value of the cooldown is 1 hour:

    // Minimum time between increasing and decreasing user share in SharedRewards contracts.
    // Prevents reward hunting where users could frontrun reward distributions and then immediately withdraw.
    // Range: 15 minutes to 6 hours with an adjustment of 15 minutes
    uint256 public modificationCooldown = 1 hours;

Now, considering that the cooldown gets extended both in _decreaseUserShare and _increaseUserShare:

    function _increaseUserShare(/*params*/) internal
        {
        require( poolsConfig.isWhitelisted( poolID ), "Invalid pool" );
        require( increaseShareAmount != 0, "Cannot increase zero share" );
        UserShareInfo storage user = _userShareInfo[wallet][poolID];
        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();
            }
        }
//unrelated functionality below
}

A malicious user could:

• Set up a keeper type of bot that increases/decreases his share with small amounts immediately after or right when the cooldown expires. Keeping himself in a perpetual cooldown state making himself unable to be liquidated.
• Or monitoring the mem pool and frontrunning a liquidation transaction with an increase/decrease of his shares, making himself unable to be liquidated.

  Tools Used

  Manual review

  Recommended Mitigation Steps

  Skip the cooldown check when calling from liquidateUser by passing false for the useCooldown parameter:

  _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false);

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #312

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes removed the grade

[c4-judge]

Picodes marked the issue as satisfactory