Zaros Part 1

Summary

TradingAccount::updateActiveMarkets fails to properly handle all scenarios of position changes, potentially leading to inconsistencies in tracking active markets and accounts with active positions.

Vulnerability Details

TradingAccount::updateActiveMarkets is responsible for maintaining the list of active markets for each trading account and the global list of accounts with active positions. Albeit, the current implementation only updates these lists in two scenarios:

1. When a new position is opened (old size is zero, new size is non-zero)

2. When an existing position is closed (old size is non-zero, new size is zero)

It fails to handle the case where a position is modified from one non-zero value to another. Some issues could come about as a reasult of this:

• If a position is opened in a transaction that modifies it multiple times, the market might not be added to the active markets list if both oldPositionSize and newPositionSize are non-zero in the final call to updateActiveMarkets.

• If a position is closed but due to rounding errors or very small positions, both oldPositionSize and newPositionSize are considered non-zero, the market won't be removed from the active markets list.

• The global list of accounts with active positions might become out of sync with the actual state of accounts' positions.

Look at this part of the code: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L585-L616

Impact

Check vulnerability details section

Tools Used

Manual

Recommendations

updateActiveMarkets function should be modified to handle all scenarios of position changes. We could consider updating the logic to always add the market to the active list if the new position size is non-zero, and always remove it if the new position size is zero, regardless of the old position size.