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

Off-chain orders can be intentionally failed by placing invalid order

SBSecurity

Summary

fillOffchainOrders() can be grief by anyone because it reverts on an invalid order.

Vulnerability Details

fillOffchainOrders() iterates through users orders and if an order is invalid the whole function revert. Since this function will tries to execute multiple orders that user has signed off-chain, users can always sign invalid order because this will cost them nothing and thus the whole fillOffchainOrders() will fail.

186: function fillOffchainOrders(
187: uint128 marketId,
188: OffchainOrder.Data[] calldata offchainOrders,
189: bytes calldata priceData
190: )
191: external
192: onlyOffchainOrdersKeeper(marketId)
193: {
194:
...
211:
212: // iterate through off-chain orders; intentionally not caching
213: // length as reading from calldata is faster
214: for (uint256 i; i < offchainOrders.length; i++) {
215: ctx.offchainOrder = offchainOrders[i];
216:
217: // enforce size > 0
218: if (ctx.offchainOrder.sizeDelta == 0) {
219: revert Errors.ZeroInput("offchainOrder.sizeDelta"); // AUDIT - Here
220: }
221:
...
225:
226: // enforce that keeper is filling the order for the correct marketId
227: if (marketId != ctx.offchainOrder.marketId) {
228: revert Errors.OrderMarketIdMismatch(marketId, ctx.offchainOrder.marketId); // AUDIT - Here
229: }
230:
231: // First we check if the nonce is valid, as a first measure to protect from replay attacks, according to
232: // the offchain order's type (each type may have its own business logic).
233: // e.g TP/SL must increase the nonce in order to prevent older limit orders from being filled.
234: // NOTE: Since the nonce isn't always increased, we also need to store the typed data hash containing the
235: // 256-bit salt value to fully prevent replay attacks.
236: if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
237: revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce); // AUDIT - Here
238: }
239:
240: ctx.structHash = keccak256(
241: abi.encode(
242: Constants.CREATE_OFFCHAIN_ORDER_TYPEHASH,
243: ctx.offchainOrder.tradingAccountId,
244: ctx.offchainOrder.marketId,
245: ctx.offchainOrder.sizeDelta,
246: ctx.offchainOrder.targetPrice,
247: ctx.offchainOrder.shouldIncreaseNonce,
248: ctx.offchainOrder.nonce,
249: ctx.offchainOrder.salt
250: )
251: );
252:
253: // If the offchain order has already been filled, revert.
254: // we store `ctx.hash`, and expect each order signed by the user to provide a unique salt so that filled
255: // orders can't be replayed regardless of the account's nonce.
256: if (tradingAccount.hasOffchainOrderBeenFilled[ctx.structHash]) {
257: revert Errors.OrderAlreadyFilled(ctx.offchainOrder.tradingAccountId, ctx.offchainOrder.salt); // AUDIT - Here
258: }

Impact

Any user can grief fillOffchainOrders().

Tools Used

Manual Review

Recommendations

In all these cases, instead of reverting, use continue and skip this order.

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.