Zaros Part 1

Summary

checkLowerBound, checkUpperBound, performLowerBoundand performUpperBoundare all index values but both indexes checkUpperBoundand performUpperBoundare excluded in LiquidationKeeper.sol::checkUpkeep()but the upperBoundindex is included in GlobalConfigurationBranch::getAccountsWithActivePositions.

This creates confusion as the upperBound is inconsistence accross different functions.

Vulnerability Details

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

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

In the checkLiquidatableAccountsfunction we can see that the length of the liquidateableAccountsIds is being created using upperBoundand lowerBound.
Now let's take an example:-
upperBound = 10, lowerBound = 5.
liquidatableAccountsIds = 10 - 5 which is 5.
And later we have for loop which will iterate for the index value of 5,6,7,8,9. as i < upperBound

also in checkUpkeepfunction performUpperBoundis excluded

Github:- https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L67

Impact

Account which indexed at checkUpperBound will not liquidate, although it can be liquidate later but in time sensitive scenario such as when price is fluctuating very fast we don't want any delay.

Tools Used

Manual Review

Recommendations

This will not create problem if we keep in mind that index checkUpperBound and performUpperBound is excluded in checkUpkeep and always pass the upperIndex that we want to include + 1 as arguments , but human has the tendency to messed up things, so here's a better way:-

LiquidationBranch.sol::checkLiquidatableAccounts()

LiquidationKeeper::checkUpkeep()