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

`fillOffchainOrders()` validates the price wrongly.

bladesec

Github link

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

Summary

fillOffchainOrders() validates the price wrongly.

Vulnerability Details

fillOffchainOrders() fills offchain orders by offchain order keepers.

// 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()) //@audit wrong validation
|| (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());

During a validation, it compares targetPrice with fillPrice wrongly.

Currently, it passes if targetPrice <= fillPrice for a buy order but it's wrong because the buyer should pay a higher price(fillPrice) than he has requested(targetPrice) while creating an offchain order.

The comment says correctly but the implementation is different from the comment.

Impact

There are 2 impacts.

  • User would lose funds due to the worse fill price when it should revert.

  • fillOffchainOrders() would revert when it should work properly because of the wrong price validation.

Tools Used

Manual Review

Recommendations

We should validate like the below.

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