c4-bot-6 commented 5 months ago

Lines of code

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

Vulnerability details

The CollateralAndLiquidity.liquidateUser() function calls StakingRewards._decreaseUserShare() to wipe out a user's share in the WBTC/WETH pool that they have used as collateral to borrow USDS. Because the useCooldown parameter is set to true, the call will fail if the user is currently in the cooldown period.

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

    [...]

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

    [...]
    }

// Decrease a user's share for the pool and have any pending rewards sent to them.
// Does not require the pool to be valid (in case the pool was recently unwhitelisted).
function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
    {
    [...]

    UserShareInfo storage user = _userShareInfo[wallet][poolID];

    [...]

    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 cooldown period starts when a user deposits or withdraws collateral. The cooldown period is set in the StakingConfig contract to be 1 hour by default (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L31) but can be change to a value between 15 minutes and 6 hours by the DAO (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L85-L99).

A user can avoid liquidation by depositing a minimal amount of collateral. However, when the cooldown period ends the user might need to frontrun a possible liquidation. Depending on the value of their collateral they might be financially incentivized to pay an additional gas fee or MEV bribe to ensure that the liquidation fails.

Impact

The protocol risks bad debt due to delayed or failed liquidations if the collateral value minus the liquidation reward falls below the value of the borrowed USDS while the liquidation is blocked. This can also affect the USDS peg given that USDS is backed by the collateral provided by the borrowers.

Proof of Concept

Add the following test to https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/tests/CollateralAndLiquidity.t.sol:

function testUserDepositBorrowDepositAndLiquidateBlockedByCooldown() 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 );

  vm.warp( block.timestamp + 1 days );

  // Try depositing again
  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
  vm.stopPrank();

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

  // Alice deposits minimal WBTC/WETH collateral to start the cooldown period
  vm.prank(alice);
  collateralAndLiquidity.depositCollateralAndIncreaseShare(1000, 1000, 0, block.timestamp, true );
  assertTrue(_userHasCollateral(alice));
  // Alice is liquidatable
  assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

  // Liquidate Alice's position - fails because Alice is in cooldown
  vm.expectRevert( "Must wait for the cooldown to expire" );
  collateralAndLiquidity.liquidateUser(alice);
}

Run the test with:

COVERAGE=no forge test --match-path src/stable/tests/CollateralAndLiquidity.t.sol --match-test testUserDepositBorrowDepositAndLiquidateBlockedByCooldown -vvv --fork-url https://eth-sepolia-public.unifra.io

A fork URL (Sepolia RPC endpoint) and the COVERAGE env variable must be specified for the test to work.

Recommended Mitigation Steps

CollateralAndLiquidity.liquidateUser() should call _decreaseUserShare() with useCooldown set to false. This ensures that liquidations cannot be blocked by the cooldown period. The cooldown mechanism should not be needed in this case.

Assessed type

Other

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #891

c4-judge commented 4 months ago

Picodes marked the issue as satisfactory