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

Improper Bounds Handling in `checkLiquidatableAccounts` Leading to Potential Liquidation Bypass

pelz

Summary

The checkLiquidatableAccounts function in the contract does not handle the lower bound properly when it is not zero. This can result in accounts not being liquidated when they should be, due to an array out-of-bounds error in the function's loop.

Vulnerability Details

The checkLiquidatableAccounts function is designed to return a list of liquidatable accounts within a specified range (lowerBound to upperBound). However, when the lower bound is not zero, the function miscalculates the size of the liquidatableAccountsIds array and can lead to an out-of-bounds error. This issue stems from the array initialization and the subsequent indexing logic within the for loop.

Detailed Explanation

The function initializes the liquidatableAccountsIds array with the size upperBound - lowerBound. However, within the for loop, the array is indexed directly using i, which starts from lowerBound. This causes incorrect indexing and potential array out-of-bounds errors, preventing the function from correctly identifying and returning liquidatable accounts.

Code Snippet

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 - lowerBound] = tradingAccountId; // Fix indexing issue here
}
}
}

Proof of Concept

Consider a scenario where lowerBound is 10 and upperBound is 20. The array liquidatableAccountsIds is initialized with a size of 10 (20 - 10). When the for loop starts with i = 10, it tries to assign to liquidatableAccountsIds[10], causing an out-of-bounds error since the array only has indices from 0 to 9.

Impact

The vulnerability allows accounts that should be liquidated to bypass liquidation checks, potentially leading to significant financial risk on the platform. If liquidatable accounts are not identified correctly, they might continue operating with insufficient margin, exacerbating losses and undermining the integrity of the platform's liquidation mechanism.

Additionally, if users do not get liquidated when they should, they could potentially bypass liquidation if the price moves in their favor within the period they ought to be liquidated. This could result in users avoiding liquidation and the protocol incurring bad debt, thereby jeopardizing the financial stability of the entire system.

Tools Used

Manual review

Recommendations

  1. Bounds Checking: Ensure that the function correctly checks and processes the bounds to prevent out-of-bounds errors.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 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.

Give us feedback!