Summary

In GlobalConfigurationBranch, function removeCollateralFromLiquidationPriorityis due to removing collateral from collateralLiquidationPriorityarray. Even if protocol doesn't have any bad intention by removing collateral from collateralLiquidationPriority array, there can following cases where protocol team needs to remove collateral from collateralLiquidationPriorityarray:

1) Protocol team needs to add new collateral and it's position in collateralLiquidationPriorityarray is not at last element

2) Protocol team needs to change the order of elements of collateralLiquidationPriorityarray

In above two cases, protocol team will need to momentarily remove collateralfrom collateralLiquidationPriorityarray and vulnerabilities that can occur due to this is explained below:

Vulnerability Details

When collateral is excluded from thecollateralLiquidationPriorityarray, it won't be sold off to cover the trader's debts during a liquidation event. However, the collateral value is still considered in calculating account margin. Due to this, when an account is going to be liquidated, the loss to be recovered by calling deductMarginwill not consider the collateralwhich is removed. But the calculation of margin of protocol was also including the collateral removed. This means that even if a trader's position is liquidated, the full amount owed might not be recovered because some collateral is off-limits. This can lead to significant losses for the protocol and its participants, as the protocol might need to cover the shortfall. Also, the positions which are near to be liquidated can't add the collateral to the margin if it's removed temporarily for changing order of collateralLiquidationPriorityarray.

Related code snippets:
Only considers thecollateralLiquidationPriorityarray while margin deduction https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L505-L508

Considering all the deposited collateral in calculation of marginhttps://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L178C55-L181

function. to remove collateral from collateralLiquidationPriorityarray https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/GlobalConfiguration.sol#L119

Impact

If a trader's position is liquidated, the full amount owed might not be recovered because some collateral is off-limits. This can lead to significant losses for the protocol and its participants, as the protocol might need to cover the shortfall.

Tools Used

Manual review

Recommendations

If the protocol team has plan to remove the collateral once it has been enabled previously, then margincalculation should only take into consideration the collateral which can be liquidated

If the protocol team doesn't have plan to remove the collateral but only need to do that in case of updating collateralLiquidationPriorityarray, then the method should be added to ensure that every addition and removal is done in one go without affecting other positions.