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

`checkLiquidatableAccounts()` would revert if `lowerBound` is positive.

bladesec

Github link

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L83

Summary

checkLiquidatableAccounts() would revert if lowerBound is positive.

Vulnerability Details

In checkLiquidatableAccounts(), the length of liquidatableAccountsIds is upperBound - lowerBound.

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 wrong index
}
}
}

But while iterating from lowerBound to upperBound, it updates liquidatableAccountsIds[i] directly which will revert if i >= upperBound - lowerBound.

Impact

checkUpkeep() would revert when lowerBound > 0.

Tools Used

Manual Review

Recommendations

checkLiquidatableAccounts() should use a correct index while updating liquidatableAccountsIds.

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.