Zaros Part 1

Summary

The performLowerBound parameter in the LiquidationKeeper::checkUpkeep function is used for two distinct purposes: (1) as a threshold for determining if upkeep is needed (2) and as a starting index for selecting accounts to be liquidated.
This will lead to skip liquidation of liquidatable accounts which indexes are below performLowerBound

Vulnerability Details

1. performLowerBound is first used as a threshold to determine if upkeep is needed:

   File: src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol

   61: ⚠ if (liquidatableAccountsIds.length == 0 || liquidatableAccountsIds.length <= performLowerBound) {

   62: performData = abi.encode(accountsToBeLiquidated);

   63:

   64: return (upkeepNeeded, performData);

   65: }

2. It is then used as the starting index for selecting accounts to be liquidated:

   File: src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol

   67: uint256 boundsDelta = performUpperBound - performLowerBound;

   68: uint256 performLength =

   69: boundsDelta > liquidatableAccountsIds.length ? liquidatableAccountsIds.length : boundsDelta;

   70:

   71: accountsToBeLiquidated = new uint128[](performLength);

   72:

   73: for (uint256 i; i < performLength; i++) {

   74: ⚠ uint256 accountIdIndexAtLiquidatableAccounts = performLowerBound + i;

   75: if (accountIdIndexAtLiquidatableAccounts >= liquidatableAccountsIds.length) {

   76: break;

   77: }

   78:

   79: accountsToBeLiquidated[i] = liquidatableAccountsIds[accountIdIndexAtLiquidatableAccounts];

   80: if (!upkeepNeeded && liquidatableAccountsIds[accountIdIndexAtLiquidatableAccounts] != 0) {

   81: upkeepNeeded = true;

   82: }

   83: }

These two purposes are logically distinct and not related.
Using the same parameter for both purposes will lead to unexpected behavior.
Example:

1. performLowerBound = 3 and performUpperBound = 10

2. checkLiquidatableAccounts returns an array of 4 accounts all liquidatables

3. boundsDelta = 7 and so bounded to 4 because of liquidatableAccountsIds.length

4. the first accessed index inside the for loop is accountIdIndexAtLiquidatableAccounts = performLowerBound + i = 3

5. as liquidatableAccountsIds.length = 4, only the last element will be accessed liquidatableAccountsIds[3]

6. the 3 other accounts are not liquidated

Impact

Unintended skipping of liquidatable accounts.

Tools Used

Manual review

Recommendations

Separate the two concerns into distinct parameters, performLowerBound to decide if upkeep is needd, and startIndex which could simply be the first element of liquidatableAccountsIds
It also seems that boundsDelta could simply be performUpperBound, as this would restrict the upkeep to this maximum value of iteration.