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

`checkLiquidatableAccounts()` returns incorrect result

T1MOH

Summary

Function LiquidationBranch.checkLiquidatableAccounts() is used by Keeper to perform health checks. This function returns array of accountIds which Keeper then liquidates.

Problem is that it uses incorrect index to populate that array of accountIds.

Vulnerability Details

Here you can see that loop variable i has prefix of lowerBound. However in the end it forgets to account prefix:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L83

function checkLiquidatableAccounts(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory liquidatableAccountsIds)
{
// prepare output array size
@> liquidatableAccountsIds = new uint128[](upperBound - lowerBound);
​
...
​
// iterate over active accounts within given bounds
@> for (uint256 i = lowerBound; i < upperBound; i++) {
...
​
// account can be liquidated if requiredMargin > marginBalance
if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
@> liquidatableAccountsIds[i] = tradingAccountId;
}
}
}

So basically consider scenario:

  1. Keeper is configured to check accounts in bounds lowerBound = 5 and upperBound = 10.

  2. liquidatableAccountsIds array will have length 10 - 5 = 5.

  3. In the very first operation it will try to set value to index 5, so it will revert due to out-of-bounds error.

Impact

As you can see checkLiquidatableAccounts() will revert when lowerBound > 0 - this is intended usage. Liquidation Keeper uses this function to track whether accounts are unhealthy:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L58

So as a result Keeper fails to track liquidateable accounts. Inability to track liquidateable accounts is obviously High risk issue.

Tools Used

Manual Review

Recommendations

if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
- liquidatableAccountsIds[i] = tradingAccountId;
+ liquidatableAccountsIds[i - lowerBound] = tradingAccountId;
}
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.