Collateral positions within cooldown period can not be liquidated, which may lead to an undercollateralized loans

[c4-bot-3]

Lines of code

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

Vulnerability details

Adding liquidity and providing collateral to the salty.io protocol triggers a cooldown timer which prevents adding or removing new liquidity/collateral within one hour. This cooldown time can be adjusted by the DAO in the range of 15 min to 6 hours.

Collateralized loans of USDS have a minimumCollateralRatioPercent of 110% (adjustable by the DAO in the range from 110% - 120%).

An outside entity is able to call CollateralAndLiquidity:liquidateUser and liquidate collateral of users that fell below 110%. liquidateUser calls the function StakingRewards:_decreaseUserShare and checks if the users collateral cooldown time has expired. In case the users collateral cooldown time has not expired the function reverts and prevents the liquidation of a liquidatable position.

Impact

In case of a market crash within the cooldown period loans may become undercollateralized due to the inability to liquidate these positions.

A malicious user could even prevent the liquidation of its position at all, in creating dust lp's with the min dust amounts of PoolUtils.DUST, every time the cooldown time expires.

Proof of Concept

The following foundry test, copied in CollateralAndLiquidity.t.sol demonstrates that liquidatable position can not be liquidated while their collateral is within the cooldown period.

function testPositionCanNotBeLiquidatedInCooldown() public {
        // Bob deposits collateral
        vm.startPrank(bob);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false
        );
        vm.stopPrank();

        vm.startPrank(alice);
        uint256 wbtcDeposit = wbtc.balanceOf(alice) / 4;
        uint256 wethDeposit = weth.balanceOf(alice) / 4;
        // Alice deposits collateral
        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true);

        // Alice borrows USDS
        uint256 maxBorrowable = collateralAndLiquidity.maxBorrowableUSDS(alice);
        collateralAndLiquidity.borrowUSDS(maxBorrowable);
        vm.stopPrank();

        _crashCollateralPrice();

        // fast forward 50 min which is within the cooldown period currently set on 1 hour
        vm.warp(block.timestamp + 50 minutes);

        assertTrue(_userHasCollateral(alice));

        assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

        // Reverts in StakingRewards:_decreaseUserShare which leads to user can not be liquidated in case cooldown period is not expired
        vm.expectRevert("Must wait for the cooldown to expire");
        collateralAndLiquidity.liquidateUser(alice);
    }

Tools Used

Foundry

Recommended Mitigation Steps

In CollateralAndLiquidity:liquidateUser _decreaseUserShare should be called with the parameter useCooldown to false. This change omits the check if the cooldown period of the provided collateral is expired or not. Hence setting useCooldown to false makes it possible to liquidate users positions at any time and prevents reverts on liquidatable positions while in cooldown period.

 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]);
        //q why is this a 2 step process?
        // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
        - _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, true);
        + _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, false);

...

Assessed type

Timing

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory