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

offChainOrders could potentially fill at the wrong prices due to wrong implementation of checks

0xd4n13l

Summary

For offchain orders to get fill according to the protocol if its a buy order the fill price must be less than or equal to the target price and if its a sell order the fill price must be greater than or equal to the target price and this isn't correctly implemented in the code.

Vulnerability Details

We refer to settlementBranch.sol function fillOffchainOrders

// 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;
}

from this portion of the code notice the comment section says the corrrect thing but the if statement for isBuyOrder being true is saying targetPrice is less than or equal to fillPrice which is contrary to the logic of the protocol and likewise the second part of the check for sellOrder is checking targetPrice is greater than or equal to fill price which is contrary to the logic of the protocol.

Impact

This could lead to buy orders being filled price greater than the target and sell orders being filled at price less than the target

Tools Used

manual review

Recommendations

- 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());
​
// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}
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.