Zaros Part 1

Summary

LiquidationBranch.liquidateAccounts does not guarantee to liquidate the riskier positions as expected.

Vulnerability Details

In the previous audit report,
there is an issue saying that "7.3.7 Liquidation leaves traders with unhealthier and riskier collateral basket, making them more likely to be liquidated in future trades".
This is not fixed yet.

TradingAccount.Data.activeMarketsIds is updated only in TradingAccount.updateMarkets.
TradingAccount.updateActiveMarkets deletes all activeMarketIds and adds remaining active market ids to keep the order of the previous self.activeMarketsIds.
It is, however, already sorted in the order of the timestamp when each position is created, not in the order of GlobalConfiguration.collateralLiquidationPriority.

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/leaves/TradingAccount.sol#L585-L617

activeMarketsIds is an Openzeppelin EnumerableSet.

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/leaves/TradingAccount.sol#L52

EnumerableSet._add just pushes a new value to the end of the array.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.0.2/contracts/utils/structs/EnumerableSet.sol#L65-L71

In LiquidationBranch.liquidateAccounts, it iterates tradingAccount.activeMarketsIds and closes positions in the order of the creation timestamp.

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/LiquidationBranch.sol#L167-L173

Impact

A liquidation does not guarantee to liquidate the riskier positions as expected.

Tools Used

Manual.

Recommendations

In LiquidationBranch.liquidateaccounts, iterate user positions in the order of GlobalConfiguration.collateralLiquidationPriority.