search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Collateral provider can not increase their collateral before cooldown expired #693

Closed c4-bot-9 closed 9 months ago

c4-bot-9 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L67

Vulnerability details

Impact

USDS borrower might be liquidated due to unable increasing collateral

Proof of Concept

Anyone can deposit WBTC and WETH into Salty Protocol as WBTC/WETH liquidity. WBTC/WETH liquidity provider is allowed to borrow USDS with WBTC/WETH liquidity as collateral. The borrower could be liquidated if their collateral ratio is under the minimum threshold.

If the prices of WBTC and WETH drop a lot, A borrower have to increase their collateral ratio by depositing WBTC/WETH to avoid to be liquidated. However, a borrower might not be able to increase their collateral on time due to cooldown restriction, resulting in liquidation.

Suppose StakingConfig.modificationCooldown is 1 hour, StableConfig.initialCollateralRatioPercent is 200 and StableConfig.minimumCollateralRatioPercent is 110:

Copy below codes to CollateralAndLiquidity.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testDepositCollateralTwiceInOneHour

  function testDepositCollateralTwiceInOneHour() public
  {
    uint wbtcBalance = wbtc.balanceOf(alice);
    uint wethBalance = weth.balanceOf(alice);
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcBalance/2, wethBalance/2, 0, block.timestamp, true );
    vm.warp(block.timestamp + 30 minutes);
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcBalance/2, wethBalance/2, 0, block.timestamp, true );
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Remove cooldownExpiration checking from _increaseUserShare():

  function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
  {
  require( poolsConfig.isWhitelisted( poolID ), "Invalid pool" );
  require( increaseShareAmount != 0, "Cannot increase zero share" );

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

  uint256 existingTotalShares = totalShares[poolID];

  // Determine the amount of virtualRewards to add based on the current ratio of rewards/shares.
  // The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool.
  // The virtual rewards will be deducted later when calculating the user's owed rewards.
  if ( existingTotalShares != 0 ) // prevent / 0
  {
    // Round up in favor of the protocol.
    uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

    user.virtualRewards += uint128(virtualRewardsToAdd);
    totalRewards[poolID] += uint128(virtualRewardsToAdd);
  }

  // Update the deposit balances
  user.userShare += uint128(increaseShareAmount);
  totalShares[poolID] = existingTotalShares + increaseShareAmount;

  emit UserShareIncreased(wallet, poolID, increaseShareAmount);
  }

Assessed type

Timing

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #963

c4-judge commented 8 months ago

Picodes changed the severity to QA (Quality Assurance)