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

`fillOffchainOrders()` shouldn't revert when it fails to fill one offchain order.

bladesec

Github link

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

Summary

fillOffchainOrders() shouldn't revert when it fails to fill one offchain order.

Vulnerability Details

While filling offchain orders, it validates several requirements and reverts if one of them is invalid.

if (ctx.offchainOrder.nonce != tradingAccount.nonce) { //@audit shouldn't revert
revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
}

Especially, it reverts if the trading account's nonce was increased which is possible by the account owner anytime.

In this case, fillOffchainOrders() will revert and other valid offchain orders won't be filled.

I think the above case should be continued without reverting like the invalid price.

// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}

Impact

fillOffchainOrders() would revert unnecessarily for some cases.

Tools Used

Manual Review

Recommendations

Recommend modifying like the below.

if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
- revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
+ continue;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

fillOffchainOrders reverts everything if a single order fails one of the multiple checks

If you send 1 cancel and 1 create it should still run the cancel, not revert everything.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.