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

checkLiquidatableAccounts range calculation is wrong

josh4324

Summary

The checkLiquidatableAccountsfunction which is called from the LiquidationKeeper contracts is not calculated correctly.

Vulnerability Details

The checkLiquidatableAccountshas two parameters uint256 lowerBound, uint256 upperBound and it should be able to take the min of 0 and the max of type(uint256).max but the function only works correctly when the the lowerBound is zero, this is due to the the way liquidatableAccountsIds is being derived, in order to get the liquidatableAccountsIds array, the function sets the size of the array by subtracting the upperBound from the lowerBound.

Test A - If the lowerbound is 0 and the upperBound is 20, the array will have a size of 20

Test B - If the lowerbound is 1 and the upperBound is 21, the array will have a size of 20

Only test A works in the checkLiquidatableAccountsfunctions even though they both have an array size of 20, the problem can be traced to the for loop which starts at 1 or any other number apart from zero.

liquidatableAccountsIds[i] = tradingAccountId;

The statement above which can be found on line 42 in the code below assumes that the lowerbound will start at zero and when the lowerbound does not start at zero, there will not be enough slots in the array for the other accounts which will lead to a panic: array out-of-bounds access (0x32) error.

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;
}
}
}

POC

Add test to the test/integration/perpetuals/liquidation-branch/checkLiquidatableAccounts/checkLiquidatableAccounts.t.sol and run it.

function test_errorWhenCheckingLiquidatableAccounts(
uint256 marketId,
bool isLong,
uint256 amountOfTradingAccounts
)
external
{
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
amountOfTradingAccounts = 30;
​
uint256 marginValueUsd = 10_000e18 / amountOfTradingAccounts;
uint256 initialMarginRate = fuzzMarketConfig.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(fuzzMarketConfig, tradingAccountId, initialMarginRate, accountMarginValueUsd, isLong);
}
​
setAccountsAsLiquidatable(fuzzMarketConfig, isLong);
​
uint256 lowerBound = 1;
uint256 upperBound = 21;
​
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(lowerBound, upperBound);
​
}
​

Impact

  1. When the liquidation keepers call the checkLiquidatableAccounts with lowerBound parameter that is not zero, it will always revert.

  2. Accounts that should be liquidated will not be liquidated on time due to the errors encountered when the keepers call the checkLiquidatableAccounts function.

Tools Used

Manual Review

Recommendations

Rewrite the checkLiquidatableAccounts logic to accomdate lowerBound that are greater than zero.

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!