Zaros Part 1

Summary

The fillOffchainOrders function is using the mark price as the fill price and then comparing it to the target price, which doesn't correctly implement the intended logic for limit orders.

Vulnerability Details

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L279C13-L292C14

The problem is that the fillOffchainOrders function is using the mark price as the fill price and then comparing it to the target price, which doesn't correctly implement the intended logic for limit orders. This can lead to several issues:

1. Incorrect price comparison: For a buy order, we want to fill if the market price is less than or equal to the target price. For a sell order, we want to fill if the market price is greater than or equal to the target price. The current implementation does the opposite.

2. Using mark price instead of market price: The mark price might not accurately reflect the current market price, especially in volatile markets.

Impact

This implementation might fill orders at prices worse than what the user specified in their limit order.

Tools Used

Manual Review

Recommendations

Use the actual market price (bid for sell orders, ask for buy orders) instead of the mark price for comparison.

• Correct the comparison logic to properly implement limit order behavior.

`ctx.marketPriceX18 = ctx.isBuyOrder ? ctx.askX18 : ctx.bidX18;

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

if (ctx.isFillPriceValid) {
// Use the market price for the fill, not the mark price
ctx.fillPriceX18 = ctx.marketPriceX18;

}`