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

`SettlementBranch.fillOffchainOrders` intends to fill TP orders, but the code is for SL orders.

qpzm

Summary

SettlementBranch.fillOffchainOrders intends to fill TP orders, but the code is for SL orders.

Vulnerability Details

If the order is a buy order, the comment says "the fill price must be less than or equal to the target price".
However, the code checks isBuyOrder && targetPrice <= fillPrice, which is the opposite of the comment.
It is the same with a sell order.

// @audit The code is the opposite of the comment below.
// 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());

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/SettlementBranch.sol#L283-L287

The comment assumes TP(Take Profit) order, but the code is for SL(Stop Loss) order.

Impact

Users place TP orders like in other order book exchanges, but the system fills them as SL orders.

Tools Used

Manual.

Recommendations

There are two solutions to choose.

  1. Change the condition of ctx.isFillPriceValid to support TP orders.

- 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());

https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/SettlementBranch.sol#L283-L287

  1. Add bool triggerAboveThreshold in OffchainOrder.Data struct to support both TP/SL orders.

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