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

Wrong Invalidation of fillPrice would lead to valid OffchainOrders being skipped

forgebyola

Summary

Legitimate offchain orders would be skipped and not filled due to a wrong validation of the fill price when filling orders. This would cause failure of the zaros operations.

Vulnerability Details

Users can sign offchain orders and these can be filled by a keeper network. During this process, a validation occurs which checks if the fill price is valid

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
.............................................................
​
for (uint256 i; i < offchainOrders.length; i++) {
.............................................................
ctx.fillPriceX18 = perpMarket.getMarkPrice(sd59x18(ctx.offchainOrder.sizeDelta), ctx.indexPriceX18);
​
//audit-issue: The implementation is inverted which would lead to inability to fill orders
// 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());
​
// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}
..............................................................
}
​

According to the code documentation, the fill price is valid if it is a buyOrder and the fillPrice is lesser or equal to the target price and if it is a sellOrder, the fillPrice is valid if it is greater or equal to the target price.
However, within the code implementation itself, the opposite is implemented. Causing fillPrice to be set to valid when the opposite is the case.
When it is invalid, the order is skipped and is not filled.
In this case, if offchain orders are created in the correct way as documented, they would be set to invalid and would be skipped.

Impact

Valid offchain orders would unnecessarily be skipped due to this wrong validation causing failure of normal processing of filling offchain orders. Users may become griefed and possibly lose funds.

Tools Used

Manual Review

Recommendations

Change the implementation to be as in the documentation which is the correct mechanism

- ctx.isFillPriceValid = (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 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.