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

off-chain orders have wrong fill and trade price checks

SBSecurity

Summary

Off-chain order has a wrong targetPrice check which will skip order settlements.

Vulnerability Details

The order can be fulfilled in 2 ways on-chain and off-chain. When the user wants his order to be fulfilled off-chain, he must sign a message with the required fields. One of them is targetPrice, the minimum/maximum price he is willing to pay based on the Short/Long order type.

In the code, if the order is Long (BuyOrder), it checks if targetPrice ≤ fillPrice, otherwise skip that order. For Short orders, it checks the opposite targetPrice ≥ fillPrice.

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

But as you can see the comment above the code says the opposite and the comment is correct. When the order is Long, it should check targetPrice ≥ fillPrice because fillPrice is the price when the order is filled and targetPrice is the maximum price the user wants their order to be filled at, so if targetPrice is $100, fillPrice should be $100 or less. The user places a Buy order, he thinks the price will go up, so he has to set a maximum price at which he wants his order to be filled, anything above that can hurt him, because the price will never go up forever.

If he wants to open a Long position believing that the price will go from X to 2X, he will put a number between those two for targetPrice so that he can be in profit.

The same check is implemented in the Synthetix code. There, if fillPrice > desireFillPrice for Long, the function will revert, but in the Zaros code it's the opposite (isFillPriceValid will be true).

PerpsV2MarketProxyable.sol#L98-L108

function _assertFillPrice(
uint fillPrice,
uint desiredFillPrice,
int sizeDelta
) internal view returns (uint) {
_revertIfError(
sizeDelta > 0 ? fillPrice > desiredFillPrice : fillPrice < desiredFillPrice,
Status.PriceImpactToleranceExceeded
);
​
// long - fill > desired - revert
// short - full < desired - revert
​
return fillPrice;
}

Impact

Orders applying the correct target prices will never be executed or will be executed at a less advantageous price that does not satisfy the trader.

Tools Used

Manual Review

Recommendations

Flip the operators.

// 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());
+ 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 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.