Zaros Part 1

Summary

There is an inconsistency between the comment and the implementation of the order execution logic in the SettlementBranch::fillOffchainOrders function. The comment describes the desired behavior for filling orders, but the code implements the opposite logic.
Link: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L283-L287

Vulnerability Details

The SettlementBranch::fillOffchainOrders function fills pending, eligible offchain orders targeting the given market id. The problem is that the function should ensure that 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. But the code actually does the opposite:

In the contest's README it is written:

But in the SettlementBranch::fillOffchainOrders function there is a comment:

But the current code logic is that: for buy order, the fill price must be greater than or equal to the target price. And for the sell order, the fill price must be less than or equal to the target price.
That is the opposite of the comment and because the repo code and comments in it are the final word on functionality, there is an issue in the code. The current version of the code doesn't implement the required functionality.

Impact

The isFillPriceValid will be true, for example, in the case when the order increases the trading account's position and the fill price is greater than or equal to the target price. Also, it will be true in the case when the order decreases the trading account's position and the fill price is less than or equal to the target price.

But according to the the comment above the code the isFillPriceValid must be true in the cases: when the order increases the trading account's position and the fill price is less than or equal to the target price or when the order decreases the trading account's position and the fill price is greater than or equal to the target price.
The incorrect order execution leads to orders that might be executed at prices higher than the target price, leading to potential overpayment (for buy orders) and orders that might be executed at prices lower than the target price, leading to potential underselling (for sell orders).

Tools Used

Manual Review

Recommendations

Change the code to align the comment above: