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

`LiquidationBranch.checkLiquidatableAccounts()` reverts if lowerBound is not 0.

baz1ka

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L51

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L82-L84

Summary

Because of incorrect conditions for interaction with liquidatableAccountsIds array, code can try to get element by index that is out of range resulting in reverting.

Impact

LiquidationBranch.checkLiquidatableAccounts() is an important function for perp engine as it allows for keeper to get liquidatable accounts so then they can be liquidated. Without proper functioning of this function, liquidating of unhealthy accounts can not be possible which can lead to loss for the protocol.

Proof of Concept

LiquidationBranch.checkLiquidatableAccounts() create an array that contains ids of liquidatable trading accounts. The length of this array is the difference between upper and lower bounds:

liquidatableAccountsIds = new uint128[](upperBound - lowerBound);

After that, it iterates through provided amount of accounts (upperBound - lowerBound) and checks if this account is liquidatable:

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

If an account is liquidatable then current trading account id is set to liquidatableAccountsIds. Index out of bounds error can occur if lowerBound is not 0. It would happen if liquidatableAccountsIds length is less than current index i and current account is liquidatable then it would try to set current account as liquidatable in array.

Lets take an example. Protocol has 10 accounts. First 8 are healthy, other 2 are liquidatable. Keeper tries to call this function with lower bound 5 and upper bound 10. In that case liquidatableAccountsIds length would be upperBound - lowerBound => 10 - 5 = 5. After iterating to 9th account (which is 4th in loop) index i would be 9. After checking TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18) that will return true code will try to set current tradingAccountId to liquidatableAccountsIds array with current index, but because length of array is 5 and index is 9 transaction would revert.

Coded PoC

Modify test/integration/perpetuals/liquidation-branch/checkLiquidatableAccounts/checkLiquidatableAccounts.t.sol test with next code and execute this test with forge test --match-test testFuzz_OutOfBoundErrorWhenLowerBoundIsNotZero:

function testFuzz_OutOfBoundErrorWhenLowerBoundIsNotZero(uint256 marketId, uint256 amountOfHealthyAccounts, uint256 amountOfLiquidatableAccounts, uint256 lowerBound) external {
amountOfHealthyAccounts = bound(amountOfHealthyAccounts, 5, 15);
amountOfLiquidatableAccounts = bound(amountOfLiquidatableAccounts, 1, 4);
lowerBound = bound(lowerBound, 1, amountOfHealthyAccounts - amountOfLiquidatableAccounts);
​
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
​
uint256 totalAmountOfTradingAccounts = amountOfHealthyAccounts + amountOfLiquidatableAccounts; // TOTAL
​
uint256 marginValueUsd = 100_000e18 / totalAmountOfTradingAccounts;
uint256 initialMarginRate = fuzzMarketConfig.imr;
uint256 accountMarginValueUsd = marginValueUsd / totalAmountOfTradingAccounts;
​
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
​
// Create healthy positions
for (uint256 i; i < amountOfHealthyAccounts; i++) {
uint128 tradingAccountId = createAccountAndDeposit(accountMarginValueUsd, address(usdz));
openPosition(fuzzMarketConfig, tradingAccountId, initialMarginRate, accountMarginValueUsd, false);
}
​
// Create liquidatable positions
for (uint256 i; i < amountOfLiquidatableAccounts; i++) {
uint128 tradingAccountId = createAccountAndDeposit(accountMarginValueUsd, address(usdz));
openPosition(fuzzMarketConfig, tradingAccountId, initialMarginRate, accountMarginValueUsd, true);
}
​
setAccountsAsLiquidatable(fuzzMarketConfig, true); // Make long positions liquidatable
​
uint256 upperBound = totalAmountOfTradingAccounts;
​
vm.expectRevert(stdError.indexOOBError);
perpsEngine.checkLiquidatableAccounts(lowerBound, upperBound);
}

Recommended Mitigation Steps

Specifically this error can be prevented by checking as part of this condition to check whether current index is greater or equal to liquidatableAccountsIds array length but this would make function unusable as with large lowerBound (where lowerBound >= upperBound - lowerBound) it would just return an empty array. I would recommend rewriting function where you first get amount of liquidatable accounts, then create array with length of liquidatable accounts and then iterate to fill this array.

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.