Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

The `checkLiquidatableAccounts` function will be reverting often

draiakoo

Summary

The checkLiquidatableAccounts function will be reverting often

Vulnerability Details

The checkLiquidatableAccounts is a view function that is really important for the protocol because is the function checker that the keepers will call in order to know which accounts to liquidate. If this function does not work properly will result in a high impact issue that may lead the protocol to a state of insolvency because of the liquidator keepers not being able to liquidate.
This view function does the following:

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++) {
// 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;
}
}
}

The function requires a lowerBound and an upperBound to avoid looping through a huge array that uses all gas, and what it does is to go through the accounts with active positions from the global array within these bounds computing if their position is liquidatable or not. After that it should account the unhealthy positions to an array that will be returned.
However, this function is likely to revert due to accessing a position of an array that exceeds its length. Let's take a look of an example:

  • The accountsIdsWithActivePositions has 1000 positions in it

  • The selected lowerBound is 600 and the higherBound is 900

The function will create the liquidatableAccountsIds array with 900-600=300 elements in it. This will be the function that should contain the liquidatable accounts. Now let's imagine that the first position checked in the loop is already liquidatable, it will enter the last if statement and it will try to assign in the position 600 of the liquidatableAccountsIds the trading account ID. Since the array was initialized with only 300 elements, the position 600 will not exist and it will revert.

Impact

The impact of this issue is high because it is a function that will be used by the liquidation keepers to determine which accounts to liquidate, so if this function reverts, there will be no liquidations executed and the protocol can get to a state of insolvency.

Tools Used

Manual review

Recommendations

Following the same structure of leaving empty spaces in between because they will be ignored later when the liquidateAccounts will be executed, I would recommend the following:

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++) {
// 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;
+ liquidatableAccountsIds[i - lowerBound] = tradingAccountId;
}
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`LiquidationBranch::checkLiquidatableAccounts()` executes `for` loop where `liquidatableAccountsIds[i] = tradingAccountId;` gives out of bounds if `lowerBound != 0`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.