Tiny collateral share increase prevents liquidation

[c4-bot-4]

Lines of code

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

Vulnerability details

Title

Tiny collateral share increase prevents liquidation

Links to affected code

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

Vulnerability details

Impact

To maintain the invariant of USDS overcollateralization, it is imperative to permit the liquidation of positions that fall below the required collateral threshold.

When a user becomes undercollateralized due to market fluctuations, they can prevent their liquidation by using the modification cooldown, that serves as a temporary shield against liquidation, providing a window during which no liquidation actions can be taken.

A user can continually prevent liquidation by adding a minimal amount of collateral at the conclusion of each cooldown period, effectively granting an indefinite reprieve from liquidation.
Transaction ordering could allow for a liquidation transaction to precede the collateral increase, but the presence of a cap on liquidation rewards introduces an economic factor, incentivizing liquidators to moderate their gas expenditures to ensure that the liquidation remains financially viable.

By default, the reward cap is only $500 USD. To order share increase before any liquidations, the gas price merely needs to push the liquidation transaction cost above that.
With a constant cost in execution, the strategy becomes more optimal the larger the share (the larger the amount of USDS undercollateralized), which are the holding you need liquidated as they have the greatest impact on overall collateralization.

To push the liquidation transaction gas cost above $500 USD with the increase share gas is ~$667 USD, proportionally a trivial sum for a seriously large holding.

Proof of Concept

Steps

1. Deposit collateral
2. Mint maximum USDS
3. Wait
4. Position undercollateralized
5. Deposit tiny amount of collateral
6. Await end of cooldown period and repeat step 5.

Invariant: USDS is overcollateralized

The Salty.io documentation

USDS is the overcollateralized stablecoin native to the Salty.IO ecosystem which is pegged to the US Dollar and uses deposited WBTC/WETH LP as collateral.

Tiny increase of collateral is allowed

In the flow for CollateralAndLiquidity::depositCollateralAndIncreaseShare(), minimum values are only required by Pools::addLiquidity() and StakedRewards::increaseUserShare().

The amount of both tokens must be greater than dust, in Pools::addLiquidity()

		require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
		require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

The share must be greater than zero, in StakingRewards::_increaseUserShare()

		require( increaseShareAmount != 0, "Cannot increase zero share" );

Modification Cooldown period

Default of 1 hour, with potential range of 15 minutes to 6 hours, in adjustment of 15 minutes, in StakingConfig

	// Minimum time between increasing and decreasing user share in SharedRewards contracts.
	// Prevents reward hunting where users could frontrun reward distributions and then immediately withdraw.
	// Range: 15 minutes to 6 hours with an adjustment of 15 minutes
	uint256 public modificationCooldown = 1 hours;

Change in collateral share sets cooldown

When a user increase their share in CollateralAndLiquidity, the cooldown is updated in StakingRewards

CollateralAndLiquidity::depositCollateralAndIncreaseShare()

        // Have the user deposit the specified WBTC/WETH liquidity and increase their collateral share
    (addedAmountWBTC, addedAmountWETH, addedLiquidity) = _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping );

    emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);

Liquidity::_depositLiquidityAndIncreaseShare()

		// Increase the user's liquidity share by the amount of addedLiquidity.
		// Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive)
		// _increaseUserShare confirms the pool as whitelisted as well.
		_increaseUserShare( msg.sender, poolID, addedLiquidity, true );

CollateralAndLiquidity::_depositLiquidityAndIncreaseShare()

        // Have the user deposit the specified WBTC/WETH liquidity and increase their collateral share
    (addedAmountWBTC, addedAmountWETH, addedLiquidity) = _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping );

    emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);

StakingRewards::_increaseUserShare()

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

Modification Cooldown prevents liquidation

To liquidate a user, their share must be decreased, but when decreasing the user's share, true is given for using the cooldown.
When block.timestamp is within the modified cooldown window the transaction reverts.

CollateralAndLiquidity::liquidateUser()

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

Reward cap

Liquidation rewards are capped, initially at $500 USD.

Liquidation rewards are DAO configurable parameter in StableConfig

	// The maximum reward value (in USD) that a user receives for instigating the liquidation process.
	// Range: 100 to 1000 with an adjustment of 100 ether
  uint256 public maxRewardValueForCallingLiquidation = 500 ether;

Reward capping being performed in CollateralAndLiquidity::liquidateUser()

		// Make sure the value of the rewardAmount is not excessive
		uint256 rewardValue = underlyingTokenValueInUSD( rewardedWBTC, rewardedWETH ); // in 18 decimals
		uint256 maxRewardValue = stableConfig.maxRewardValueForCallingLiquidation(); // 18 decimals
		if ( rewardValue > maxRewardValue )
			{
			rewardedWBTC = (rewardedWBTC * maxRewardValue) / rewardValue;
			rewardedWETH = (rewardedWETH * maxRewardValue) / rewardValue;
			}

		// Reward the caller
		wbtc.safeTransfer( msg.sender, rewardedWBTC );
		weth.safeTransfer( msg.sender, rewardedWETH );

Gas Usage

A successful CollateralAndLiquidity::depositCollateralAndIncreaseShare() uses ~134% more gas than a successful CollateralAndLiquidity::liquidateUser().

Gas Report

Running 1 test for src/stable/tests/CollateralAndLiquidity.t.sol:TestCollateral
[PASS] test_liquidation() (gas: 1111192)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.28ms
| src/stable/CollateralAndLiquidity.sol:CollateralAndLiquidity contract |                 |        |        |        |         |
|-----------------------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                                       | Deployment Size |        |        |        |         |
| 4650324                                                               | 25779           |        |        |        |         |
| Function Name                                                         | min             | avg    | median | max    | # calls |
| borrowUSDS                                                            | 104722          | 123372 | 123372 | 142022 | 2       |
| depositCollateralAndIncreaseShare                                     | 127771          | 179886 | 157771 | 254118 | 3       |
| liquidateUser                                                         | 94333           | 142333 | 142333 | 190333 | 2       |
| liquidizer                                                            | 296             | 296    | 296    | 296    | 1       |
| maxBorrowableUSDS                                                     | 14680           | 27430  | 27430  | 40180  | 2       |

Add the test case to src/stable/tests/CollateralAndLiquidity.t.sol

	// Minimal liquidation test for clocking gas usage, 2 liquidations to include storage warm up in metrics
	function test_liquidation() external {
		vm.startPrank(bob);
		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );

		_depositHalfCollateralAndBorrowMax(alice);
		_crashCollateralPrice();
		vm.warp( block.timestamp + 1 days );
		collateralAndLiquidity.liquidateUser(alice);

		_depositHalfCollateralAndBorrowMax(alice);
		_crashCollateralPrice();
		vm.warp( block.timestamp + 1 days );
		collateralAndLiquidity.liquidateUser(alice);
	}

To include the gas report add this to foundry.toml

gas_reports = ["CollateralAndLiquidity"]

When running the test add --gas-report to the command to generate the gas usage table.

Test Case

Test Case

Demonstrates the increase of collateral share by a tiny amount prevents liquidation.

Add the test case to src/stable/tests/CollateralAndLiquidity.t.sol

	function test_preventing_liquidation_using_cooldown() external {
    // Tiny deposit must still be larger than DUST after zapping
    uint256 tinyDeposit = 2*PoolUtils.DUST;

    // Bob deposits collateral to keep pool reserves above DUST after Alice's liquidation
    vm.startPrank(bob);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );

    // Alice will deposit collateral and mints maximum allowed USDS
    _depositHalfCollateralAndBorrowMax(alice);

    // Artificially crash the collateral price
    _crashCollateralPrice();

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

    // Alice changes the cooldown semaphore by increases their share
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(tinyDeposit, tinyDeposit, 0, block.timestamp, true );

    // Alice can still be liquidated
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice), "Alice can be liquidated");

    // Liquidation is prevent due to cooldown
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
}

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

Allow liquidation irrespective of the user's cooldown status.

In CollateralAndLiquidity ::liquidateUser()

		// 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.
-		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );
		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

		// The caller receives a default 5% of the value of the liquidated collateral.
		uint256 rewardPercent = stableConfig.rewardPercentForCallingLiquidation();

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #891

[c4-judge]

Picodes marked the issue as satisfactory