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

Flawed order filling mechanism in `fillOffChainOrders` causes incorrect price validation and missed trades

0xshoonya

Summary

The fillOffchainOrders function has a critical logic error in its price validation mechanism. This error results in orders being skipped when they should be filled and potentially allowed to be filled when they shouldn't, contrary to the intended behavior.

Vulnerability Details

The fillOffchainOrders function in SettlementBranch.sol is designed to fill pending, eligible off-chain orders targeting the given market. The function iterates through all off-chain orders and for each order verify the provided price data against the verifier and then get the mark price based on the returned index price.

ctx.fillPriceX18 = perpMarket.getMarkPrice(sd59x18(ctx.offchainOrder.sizeDelta), ctx.indexPriceX18);

Then the function proceeds to check if the fill price is valid or not. It tries to check the following condition (specified in comments):

If the order is a buy order: fill price must be less than or equal to the target price.
If the order is a sell order: fill price must be greater than or equal to the target price.

The issue is, this check is implemented incorrectly:
SettlementBranch.sol#L286-L287

ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256())
|| (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());

  • For buy orders, the current implementation allows orders to be filled at prices higher than the user's target price. This means users could end up paying more than they intended.

  • For sell orders, the current implementation allows orders to be filled at prices lower than the user's target price. This means users could end up receiving less than they intended.

Moreover, when ctx.isFillPriceValid is false (which, due to the bug, happens when the price is favorable to the user), the function skips filling that particular order and continues to the next one.
SettlementBranch.sol#L290-L292

if (!ctx.isFillPriceValid) {
continue;
}

The bug now prevents orders from being filled when they should be filled.

  • For buy orders: Even when the market price is lower (better) than the user's target price, the order won't be filled.

  • For sell orders: Even when the market price is higher (better) than the user's target price, the order won't be filled.

Impact

Likelihood: High

Impact: High

The reversed order filling mechanism in the fillOffchainOrders function results in unfavorable conditions for users. Specifically, the current implementation skips orders that are favorable to the user and fills orders that are unfavorable. This affects all offchain orders processed by the keeper leading to significant financial losses and missed trading opportunities for users.

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.fillPriceX18.intoUint256() <= ctx.offchainOrder.targetPrice)
+ || (!ctx.isBuyOrder && ctx.fillPriceX18.intoUint256() >= ctx.offchainOrder.targetPrice);
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months 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.