Zaros Part 1

An issue was identified in the Zaros perpetual contract system where the logic for validating the fill price of offchain orders does not correctly enforce the intended price constraints according to the comments. This flaw allows orders to be filled at prices that should not be valid according to the specified target prices in buy and sell orders.

​

In the `fillOffchainOrders()` function of the `SettlementBranch.sol` contract, the condition intended to validate the fill price against the target price is reversed. The condition checks if the target price is less than or equal to the fill price for buy orders and if the target price is greater than or equal to the fill price for sell orders. This is opposite to the intended logic where:

​

- Buy orders should only be filled if the fill price is less than or equal to the target price.

- Sell orders should only be filled if the fill price is greater than or equal to the target price.

​

We can view this in the followings lines of comments and code

// if the order increases the trading account's position (buy order), the fill price must be less than or

// equal to the target price, if it decreases the trading account's position (sell order), the fill price

// must be greater than or equal to the target price.

​

ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256())

|| (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());

​

// we don't revert here because we want to continue filling other orders.

if (!ctx.isFillPriceValid) {

}

​

This code intends to skip the filling if the `FillPrice` is not valid and move to the next order but in this scenario such orders will not be skipped and the order will be filled, the `ctx.fillPriceX18` variable is then passed into the next function i.e `_fillOrder()` at the end of the current function.

​

This fillPriceX18 is used in multiple places which are

ctx.orderFeeUsdX18 = perpMarket.getOrderFeeUsd(sizeDeltaX18, fillPriceX18);

// skip

ctx.pnlUsdX18 = oldPosition.getUnrealizedPnl(fillPriceX18).add(oldPosition.getAccruedFunding(ctx.fundingFeePerUnitX18));

// skip

ctx.newPosition = Position.Data({

// skip

​

Since the checks performed in the earlier function `fillOffchainOrders()` are not valid, the new order can result in wrong `orderFeeUsdX18`, `pnlUsdX18`, `newPosition` to be corrupt.

​

Due to this reversed condition, traders could potentially execute trades at non-optimal prices, affecting the fair market trading conditions and possibly leading to manipulation or unintended financial losses/gains.

​

​

The impact of this vulnerability includes potential financial loss for users who might execute trades at prices that do not reflect their intended limits, leading to poor execution relative to market conditions. Moreover, it could be exploited by malicious actors aware of the flaw to execute trades that disadvantage other traders, potentially manipulating the market.

​

- For Buyers, Orders might be filled when they expected them to be only filled when the price matches their target price.

- For Sellers, Orders might be filled at lower prices when they expected them to be only filled when the price matches their target price.

​

Impact : High

Likelihood : High

​

​

Manual Review

​

​

- ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256()) || (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());

+ ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.fillPriceX18.intoUint256() <= ctx.offchainOrder.targetPrice) || (!ctx.isBuyOrder && ctx.fillPriceX18.intoUint256() >= ctx.offchainOrder.targetPrice);

​

This ensures that the checks are performed according to the comments

​

- For buy orders: ctx.fillPriceX18 <= ctx.offchainOrder.targetPrice

- For sell orders: ctx.fillPriceX18 >= ctx.offchainOrder.targetPrice