Zaros Part 1

Summary

Position.isIncreasing() is used to check whether the user's order is increasing the position or not. However, this function does not take into account the possibility of a user position moving from short to long or long to short, allowing many important security measures to be bypassed.

Vulnerability Details

Position.isIncreasing():

Consider the following scenario:

Alice have a position with size = 5

Alice Request a order with sizeDelta = -15

Position.isIncreasing() returns false -> Protocol thinks Alice is reducing his position.

Alice now have a position with size = -10

This incorrect `isIncreasing` value can bypass many security measures and lead to many potential problems. For example:

1. Users can still create positions in disabled markets

   As long as the user has a position in that market, then he can open very large positions in the opposite direction.

   // https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L264C1-L275C10

   // both markets and settlement can be disabled, however when this happens we want to:

   // 1) allow open positions not subject to liquidation to decrease their size or close

   // 2) prevent new positions from being opened & existing positions being increased

   //

   // the idea is to prevent a state where traders have open positions but are unable

   // to reduce size or close even though they can still be liquidated; such a state

   // would severly disadvantage traders

   if (ctx.isIncreasing) {

   // both checks revert if disabled

   globalConfiguration.checkMarketIsEnabled(params.marketId);

   settlementConfiguration.checkIsSettlementEnabled();

   }

2. Users can bypass the InitialMargin and open positions using a margin between InitialMargin and MaintenanceMargin.

   1. Alice wants to open a short position with size = -1000

   2. Alice open a long position with size = 1 (Assuming more than minimum size)

   3. Alice make a short order with sizeDelta = -1001

   4. As a result of this issue, the market believes that Alice is reducing his position and therefore requires Alice to have only the MaintenanceMargin.

   //https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L325C1-L337C47

   // check maintenance margin if:

   // 1) position is not increasing AND

   // 2) existing position is being decreased in size

   //

   // when a position is under the higher initial margin requirement but over the

   // lower maintenance margin requirement, we want to allow the trader to decrease

   // their losing position size before they become subject to liquidation

   //

   // but if the trader is opening a new position or increasing the size

   // of their existing position we want to ensure they satisfy the higher

   // initial margin requirement

   ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)

   && ctx.isMarketWithActivePosition;

Impact

Likelihood: High - This vulnerability is not dependent on the market environment.

+

Impact: High - 1. Users can bypass the InitialMargin 2. Users can still create positions in disabled markets, make it impossible for markets to gracefully retire.

=

Severity: High

Tools Used

Manual review

Recommendations

Make additional check in Position.isIncreasing() to address the situation where a new position crosses 0.