Summary
The function LiquidationBranch::checkLiquidatableAccounts excludes the upperBound from the processing of the liquidatableAccountsIds:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L42-L69
Vulnerability Details
The function GlobalConfigurationBranch::getAccountsWithActivePositions accepts two input parameters lowerBound and upperBound and looks for all accounts with active positions from the lowerBound to the upperBound inclusive:
function getAccountsWithActivePositions(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory accountsIds)
{
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
@> accountsIds = new uint128[](upperBound - lowerBound + 1);
uint256 index = 0;
@> for (uint256 i = lowerBound; i <= upperBound; i++) {
accountsIds[index] = uint128(globalConfiguration.accountsIdsWithActivePositions.at(i));
index++;
}
}
The function LiquidationBranch::checkLiquidatableAccounts accepts also two input parameters lowerBound and upperBound and checks if the account is liquidatable, at the end returns the array with the ids of the liquidatable accounts:
function checkLiquidatableAccounts(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory liquidatableAccountsIds)
{
@> liquidatableAccountsIds = new uint128[](upperBound - lowerBound);
if (liquidatableAccountsIds.length == 0) return liquidatableAccountsIds;
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
uint256 cachedAccountsIdsWithActivePositionsLength =
globalConfiguration.accountsIdsWithActivePositions.length();
@> for (uint256 i = lowerBound; i < upperBound; i++) {
if (i >= cachedAccountsIdsWithActivePositionsLength) break;
@> uint128 tradingAccountId = uint128(globalConfiguration.accountsIdsWithActivePositions.at(i));
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
liquidatableAccountsIds[i] = tradingAccountId;
}
}
}
The problem is that in the GlobalConfigurationBranch::getAccountsWithActivePositions function the search for active accounts is made from lowerBound to upperBound inclusive, in the LiquidationBranch::checkLiquidatableAccounts the search for active accounts (accountsIdsWithActivePositions) is made from lowerBound to upperBound exclusive. That means the account at i equals to upperBound is omitted.
Also, in the both functions there is no check if lowerBound is not greater to upperBound. But there is almost none impact of that because the functions will revert in that case and that is an user mistake.
Impact
Excluding the upperBound in LiquidationBranch::checkLiquidatableAccounts results in missing the last account in the specified range, leading to incomplete processing and incorrect return liquidatableAccountsIds array. To ensure all accounts within the specified range are processed, the upperBound should be inclusive in the LiquidationBranch::checkLiquidatableAccounts function in the same way as in the
GlobalConfigurationBranch::getAccountsWithActivePositions function.
Tools Used
Manual Review
Recommendations
Ensure that the upperBound is taken into account in the loop:
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 - lowerBound + 1);
// 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++) {
+ 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;
}
- }
+ }
}
