Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

The judgment condition of `ctx.isFillPriceValid` in `SettlementBranch::fillOffchainOrders()` is wrong, and the result is opposite to the expected one.

joicygiore

Summary

The judgment condition of ctx.isFillPriceValid in SettlementBranch::fillOffchainOrders() is wrong, and the result is opposite to the expected one.

Vulnerability Details

// 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) {
continue;
}

The current code logic is:
For buy orders (ctx.isBuyOrder == true), the code requires that the transaction price (ctx.fillPriceX18) must be greater than or equal to the target price (ctx.offchainOrder.targetPrice). This means that the actual transaction price may be higher than the target price.
For sell orders (ctx.isBuyOrder == false), the code requires that the transaction price (ctx.fillPriceX18) must be less than or equal to the target price (ctx.offchainOrder.targetPrice). This means that the actual transaction price may be lower than the target price.
it does not match the annotation content verification.
This is contrary to the conventional trading logic. Normally:
Buy orders should require that the transaction price is not higher than the target price, that is, the transaction price should be less than or equal to the target price.
Sell orders should require that the transaction price is not lower than the target price, that is, the transaction price should be greater than or equal to the target price.

Code Snippet

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

Impact

The judgment condition of ctx.isFillPriceValid in SettlementBranch::fillOffchainOrders() is wrong, and the result is opposite to the expected one.

Tools Used

Manual Review

Recommendations

// 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());
+ ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256())
+ || (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256());
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

fillOffchainOrders inverses the comparison between `fillPrice` and `targetPrice`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.