Zaros Part 1

Summary

accountTotalUnrealizedPnlUsdX18 is not calculated correctly if the position sign is going to be flipped during filling a new order. This leads to larger calculation of unrealized loss, making the users over charged.

Vulnerability Details

When filling the order, accountTotalUnrealizedPnlUsdX18 is calculated as (ignoring accruedFunding for simplicity):

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L252
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/Position.sol#L135

In the above equation, the markPrice is calculated based on the sizeDelta and indexPrice.
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L232

It means that:

• If an old long position is going to be increased in size (buy order), accountTotalUnrealizedPnlUsdX18 would be positive because oldPosition is positive, and markPrice > lastInteractionPrice.

• If an old long position is going to be decreased in size (sell order), accountTotalUnrealizedPnlUsdX18 would be negative because oldPosition is positive, and markPrice < lastInteractionPrice.

• If an old short position is going to be increased in size (sell order), accountTotalUnrealizedPnlUsdX18 would be positive because oldPosition is negative, and markPrice < lastInteractionPrice.

• If an old short position is going to be decreased in size (buy order), accountTotalUnrealizedPnlUsdX18 would be negative because oldPosition is negative, and markPrice > lastInteractionPrice.

So far, everything is good.

The issue is when an old short position is going to change sign, in other words, when the sizeDelta is higher than the old short position size. So, the short position changes to long postion. In this case accountTotalUnrealizedPnlUsdX18 would be negative because oldPosition is negative, and markPrice > lastInteractionPrice. But, the value of accountTotalUnrealizedPnlUsdX18 is over calculated and over charges the user unfairly. Because:

While, if the account first creates an order to close the short position, and then creates another long position, the accountTotalUnrealizedPnlUsdX18 would be:

It is expected that accountTotalUnrealizedPnlUsdX18 for both cases be equal to each other because the final state of the protocol and market would be the same.

In summary, when the sign of a position is flipped, the uPnl should be considered as if the old position is closed, and then a new position is created. This is not implemented properly in the protocol.

PoC1

In the following test, the account has a short position with size -2000, and later creates and fill a long order with size 90_000. The console.log shows that accountTotalUnrealizedPnlUsdX18 is equal to -8800e18, equivalent to 8800$ loss. The final collateral balance is equal to 918090e18 equivalent to 918_090.

PoC2

In the following test, the account has a short position with size -2000, and later creates and fill two orders. First, a long order with size 2_000 to close the old short order, second, a long order with size 88_000. The console.log shows that total accountTotalUnrealizedPnlUsdX18 for both orders is equal to 0, equivalent to no loss. The final collateral balance is equal to 926884.48e18 equivalent to 926_884$.

Comparing this value with collateral balance in PoC1 shows that the difference is almost 926_884 - 918_090 = 8794$. This difference is because of the unfair uPnl calcualted in PoC1 as well as 6$ as the order/settlement fee.

Impact

• Over calculation of uPnl when the old position sign is going to be changed during filling of the new order.

Tools Used

Recommendations

When the sign of a position is changed, the uPnl should be considered as if the old position is closed, and then a new position is created.