Zaros Part 1

Summary

The upperBound account id in LiquidationBranch::checkLiquidatableAccounts() is neglected in the loop. If this account id happens to be liquidatable, it will be missed and could result in losses for the protocol.

Vulnerability Details

Let's take a look at the code, and while doing so, pay attention to the comment.

• LiquidationBranch::checkLiquidatableAccounts()

  function checkLiquidatableAccounts(

  uint256 lowerBound,

  uint256 upperBound

  )

  external

  view

  returns (uint128[] memory liquidatableAccountsIds)

  {

  // prepare output array size

  liquidatableAccountsIds = new uint128[](upperBound - lowerBound);

  ​

  // return if nothing to process

  if (liquidatableAccountsIds.length == 0) return liquidatableAccountsIds;

  ​

  // fetch storage slot for global config

  GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();

  ​

  // cache active account ids length

  uint256 cachedAccountsIdsWithActivePositionsLength =

  globalConfiguration.accountsIdsWithActivePositions.length();

  ​

  // iterate over active accounts within given bounds

  for (uint256 i = lowerBound; i < upperBound; i++) { // @audit-issue the upperBound accountId will be missed here

  // break if `i` greater then length of active account ids

  if (i >= cachedAccountsIdsWithActivePositionsLength) break;

  ​

  // get the `tradingAccountId` of the current active account

  uint128 tradingAccountId = uint128(globalConfiguration.accountsIdsWithActivePositions.at(i));

  ​

  // load that account's leaf (data + functions)

  TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);

  ​

  // get that account's required maintenance margin & unrealized PNL

  (, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =

  tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);

  ​

  // get that account's current margin balance

  SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);

  ​

  // account can be liquidated if requiredMargin > marginBalance

  if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {

  liquidatableAccountsIds[i] = tradingAccountId;

  }

  }

  }

In the for loop, the upperBound account id will be missed because of the wrong comparison operator.

The upperBound account id should have been included in the loop by replacing less than to less than or equal to.

Impact

The protocol will lose funds if the missed account id happens to be liquidatable or already in an insolvent state. This means the protocol will absorb these losses. The extent of the loss also depends on the size of the position taken.

Tools Used

Manual Review

Recommendations

Change the comparison operator from < to <=.