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

Off-Chain Limit Conditionals Are Backwards

0xlandlady

Summary

The isFillPriceValid value will be invalid because the conditional is backwards. This leads to both buys and sells being validated at unfavorable prices and, more importantly, at a price that the creator of the order explicitly did not agree to. This can result in orders being settled at bad prices, potentially causing a loss of funds for the user.

Vulnerability Details

When a user creates an order, they can set limit prices for both buys ("increases") and sells ("decreases"). The issue is that these limits are not respected when determining if the off-chain order is being filled at a valid price.

The NatSpec comment above the isFillPriceValid variable describes what should be happening:

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

Key points are:

  • When buying, the fillPrice MUST be less than or equal to the target.

  • When selling, the fillPrice MUST be greater than or equal to the target.

However, the code does exactly the opposite:

ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256()) //@audit backwards
|| (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256()); //@audit conditional needs to be flipped.

Because of this, orders will be wrongly filled when they shouldn't be and not filled when they should be.

Impact

Potential loss of funds when orders are wrongly filled or not filled.

Tools Used

Manual analysis

Recommendations

Flip the conditional to:

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