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

Not correct logic in `checkLiquidatableAccounts`

petersr

Summary

The logic in LiquidationBranch::checkLiquidatableAccounts is not working correctly and in some situation it will revert with index out of bound.

Vulnerability Details

Let's imagine that we have 20 as upperBound and 11 as lowerBound and we have for cachedAccountsIdsWithActivePositionsLength 13. The for cycle will iterate from 11 to 20, and if 11 is valid and this trading account is liquidatable, it will throw index out of bound, because i will be 11 and the array have only 9 elements.

Poc

Add this test to CheckLiquidatableAccounts_Integration_Test:

function testFuzz_index_out_of_bound(
uint256 marketId,
bool isLong
)
external
{
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
uint256 amountOfTradingAccounts = 20;
amountOfTradingAccounts = bound({ x: amountOfTradingAccounts, min: 1, max: 10 });
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 = 6;
uint256 upperBound = amountOfTradingAccounts;
​
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(lowerBound, upperBound);
​
assertEq(liquidatableAccountIds.length, upperBound - lowerBound);
for (uint256 i; i < liquidatableAccountIds.length; i++) {
// it should return an array with the liquidatable accounts ids
// print(liquidatableAccountIds[i]);
assertEq(liquidatableAccountIds[i], i + 1);
}
}

Impact

Users and keepers of the protocol will be not able to check does subset of the accounts are liquidatable. It will be still possible to query the info, but this means that the keeper need to iterate all trading accounts and this will consume a lot of gas.

Tools Used

Manual review

Recommendations

The following change can be made:

for (uint256 i = 0; i < upperBound-lowerBound; i++) {
// break if `i` greater then length of active account ids
if (i + lowerBound >= 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;
}
}

You should also adjust the logic of the checkUpkeep in the LiquidationKeeper.sol

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.