Zaros Part 1

Lines of code

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

Impact

When checking if the fillprice is valid, the wrong comparison is made. This will lead to offchain orders being filled at unfavourable prices, causing significant losses for traders. Specifically:

• Buy orders will be filled at prices higher than the trader's specified target price

• Sell orders may be filled at prices lower than the trader's specified target price

This completely undermines the purpose of limit orders, exposing traders to unexpected losses and violating their trading intentions.

Proof of Concept

The fillOffchainOrders() function is used for filling offchain orders placed by users. After some checks and calculations, the functions trys to verify if the calculated fill price is valid for the order and if the order should be executed:

The issue with this check is that it checks in the wrong direction resulting in a positive outcome if the fill price is bigger than the targetPrice for buy orders and the fillprice is smaller than the targetPrice for sell orders. This results in orders been filled at unfavourable prices and favourable prices to be rejected.

Example:

1. Alice places a buy limit order with a target price of 100 USDC

2. The market price rises to 110 USDC

3. A keeper calls fillOffchainOrders() with a fill price of 110 USDC

4. The current logic incorrectly determines this as a valid fill price (100 <= 110)

5. Alice's order is filled at 110 USDC, 10% higher than her specified limit price

This scenario results in Alice paying more than she intended, potentially leading to unexpected losses.

Recommended Mitigation Steps

Correct the price comparison logic in the fillOffchainOrders() function: