Summary
The LiquidationBranch::checkLiquidatableAccounts function will always throw because of the way the size of liquidatableAccountsIds array is being set in the function. consequently, if the lowerBound is not zero(0), no account that is due for liquidation within the range of accounts being checked will be liquidated.
Vulnerability Details
if the lowerBound of the check is any value greater than zero, the loop will always revert with an array_out_of_bound error because the size of liquidatableAccountsIds array(upperBound - lowerBound) is not used in the loop condition hence loop throws when iterator is greater than the the size of liquidatableAccountsIds array .
POC
below is a foundry test to verify the issue
function test_WhenCheckLiquidatableAccountsBreaks()
external
{
MarketConfig memory _marketConfig = marketsConfig[9];
uint256 amountOfTradingAccounts = 10;
uint256 marginValueUsd = 10_000e18 / amountOfTradingAccounts;
uint256 initialMarginRate = _marketConfig.imr;
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
for (uint256 i; i < amountOfTradingAccounts; i++) {
uint256 accountMarginValueUsd = marginValueUsd / amountOfTradingAccounts;
uint128 tradingAccountId = createAccountAndDeposit(accountMarginValueUsd, address(usdz));
openPosition(_marketConfig, tradingAccountId, initialMarginRate, accountMarginValueUsd, true);
}
setAccountsAsLiquidatable(_marketConfig, true);
uint256 lowerBound = 5;
uint256 upperBound = amountOfTradingAccounts;
vm.expectRevert();
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(lowerBound, upperBound);
}
Impact
Tools Used
manual review
foundry test
Recommendations
in LiquidationBranch::checkLiquidatableAccounts, adjust the code like this
function checkLiquidatableAccounts(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory liquidatableAccountsIds)
{
// prepare output array size
- liquidatableAccountsIds = new uint128[](upperBound - lowerBound);
+ liquidatableAccountsIds = new uint128[](upperBound);
// 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;
}
}
}
