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

Attempts to get liquidatable accounts would be broken in some instances

bauchibred

Summary

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L42-L86

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;//@audit
}
}
}

This function is used to get the liquidatable accounts, and it does this with the help of the lowerBound and the upperBound of the accounts to check.

Now would be key to note that this function is directly used while checking for up keeps in the LiquidationKeeper#checkUpkeep() , see https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L57-L65

function checkUpkeep(bytes calldata checkData)
{
..snip
uint128[] memory liquidatableAccountsIds =
perpsEngine.checkLiquidatableAccounts(checkLowerBound, checkUpperBound);
uint128[] memory accountsToBeLiquidated;
​
if (liquidatableAccountsIds.length == 0 || liquidatableAccountsIds.length <= performLowerBound) {
performData = abi.encode(accountsToBeLiquidated);
​
return (upkeepNeeded, performData);
}
​
..snip
}

Issue however is that there is a high chance the LiquidationBranch#checkLiquidatableAccounts() reverts.

This is because the lowerBound is not always going to be 0, but while preparing the output array size, this is being done liquidatableAccountsIds = new uint128[](upperBound - lowerBound);, now in a case where say lowerBound is 13 and upperBound is 23, we have our array to be liquidatableAccountsIds = new uint128[](10);.

Now while looping through to check if the accounts are liquidatable the i value being used are not reset to start from 0, but rather from lowerBound up until upperBound, i.e for (uint256 i = lowerBound; i < upperBound; i++) {.

Now where this reverts is here:

if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
liquidatableAccountsIds[i] = tradingAccountId;//@audit
}

Which is because in our example above, let's assume even the first account is liquidatable, while trying to attach the tradingAccountId to the i index in our case which would be 13, the execution is going to run into an OOB error, cause we are trying to access liquidatableAccountsIds[13] whereas the maximum length is liquidatableAccountsIds[10]

Impact

Impact is massive, this revert bubbles from LiquidationBranch#checkLiquidatableAccounts() back up to LiquidationKeeper#checkUpkeep() and all functions that query it, note that both Log and the AutomationCompatible have this as a core functionality, considering performUpkeep is heavily dependent on what's being returned by this query. CC: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/market-order/MarketOrderKeeper.sol#L162-L170.

Tools Used

Manual review

Recommendations

Consider applying these changes:

if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
- liquidatableAccountsIds[i] = tradingAccountId;
+ liquidatableAccountsIds[i - lowerBound] = tradingAccountId;
}

Or use a logic similar to what's here: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L44-L88.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year 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.