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

Incorrect Price Validation Logic for Off-Chain Orders

kildren

Relevant GitHub Links

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

Summary

For off-chain orders, it's crucial to ensure that the fill price doesn't exceed the target price for buy orders and isn't below the target price for sell orders. This is correctly stated in the code comments:

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

However, the code implementation contradicts this logic.

Vulnerability Details

Taking a buy order as an example, when the target price is lower than the actual fill price, it means the user would have to pay a higher cost than expected to complete the transaction. In this case, ctx.isFillPriceValid becomes true, and the order is not skipped but executed, which is clearly not what the user intended.

Impact

This vulnerability leads to a complete inversion of the intended functionality:

  • Reasonable trades are skipped

  • Unfavorable trades are executed

As a result, this feature becomes entirely unusable, potentially leading to significant financial losses for users who expect their orders to be filled according to their specified price limits.

Tools Used

Manual Review.

Recommendations

Modify the price comparison logic as follows:

- 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.