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

Incorrect Logic in SettlementBranch::filloffchainOrders() Filling Process

frontrunner

Summary

The current implementation of the order filling logic contains an error that impacts the execution of trades. The comments correctly state the desired behavior for filling orders based on the type of trade (buy or sell). However, the logic implemented does not match this intended behavior, leading to potential issues for traders trying to fill their orders.

Vulnerability Details

The current logic incorrectly implements the intended behavior:

  • For a buy order (ctx.isBuyOrder is true), the condition checks if the targetPrice is less than or equal to the fillPrice, which is the opposite of the intended behavior.

  • For a sell order (ctx.isBuyOrder is false), the condition checks if the targetPrice is greater than or equal to the fillPrice, which is also the opposite of the intended behavior.

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

Impact

Current Logic: The current implementation only fills orders if the targetPrice is less than or equal to the fillPrice. This condition causes several issues for traders:

  1. Unfilled Orders: Traders' orders will frequently not be filled as they intended. This is because the logic incorrectly skips orders where the trader sets the targetPrice to be greater than or equal to the fillPrice.

  2. Potential Losses: Even if a trader sets the targetPrice to be less than or equal to the fillPrice, the trade is likely to incur losses. This is contrary to the expected trading behavior, where a buy order should only be filled at a price less than or equal to the target price, and a sell order should be filled at a price greater than or equal to the target price.

By correcting the logic, we can ensure that:

  • Buy orders are filled only when the fillPrice is less than or equal to the targetPrice.

  • Sell orders are filled only when the fillPrice is greater than or equal to the targetPrice.

Tools Used

Recommendations

The logic in the SettlementBranch::filloffchainOrders()can be implemented as such

// 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());
​
// 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.