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

Users are able to delay off-chain orders fulfillment

greed

Summary

Zaros implements a 2 steps process to execute an order :

  1. order creation by the users (off-chain or on-chain)

  2. order fulfillment by the keepers

The keepers are implemented in a way they trigger the orders fulfillment at the most strategic moment possible.

The SettlementBranch::fillMarketOrder() function is able to execute 1 on-chain pending order at a time, which in case of revert would only affect this particular order.

Such order can revert in case it has been modified before it was executed to make sure the order executed is the intended one (this was subject to a valid issue in the previous Cyfrin audit).

Vulnerability Details

Zaros has implemented off-chain orders which are fulfilled using the SettlementBranch::fillOffchainOrders() function.

The issue is that this function can not only revert for multiple reason BUT takes multiple orders at the same time.

This means if something goes wrong with one of the orders and should revert, all the orders won't be able to be filled.

Here is an extract of the code that causes the issue :

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/SettlementBranch.sol#L214-270

for (uint256 i; i < offchainOrders.length; i++) {
ctx.offchainOrder = offchainOrders[i];
​
if (ctx.offchainOrder.sizeDelta == 0) {
revert Errors.ZeroInput("offchainOrder.sizeDelta");
}
​
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExisting(ctx.offchainOrder.tradingAccountId);
​
if (marketId != ctx.offchainOrder.marketId) {
revert Errors.OrderMarketIdMismatch(marketId, ctx.offchainOrder.marketId);
}
​
if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
}
​
ctx.structHash = keccak256(
abi.encode(
Constants.CREATE_OFFCHAIN_ORDER_TYPEHASH,
ctx.offchainOrder.tradingAccountId,
ctx.offchainOrder.marketId,
ctx.offchainOrder.sizeDelta,
ctx.offchainOrder.targetPrice,
ctx.offchainOrder.shouldIncreaseNonce,
ctx.offchainOrder.nonce,
ctx.offchainOrder.salt
)
);
​
if (tradingAccount.hasOffchainOrderBeenFilled[ctx.structHash]) {
revert Errors.OrderAlreadyFilled(ctx.offchainOrder.tradingAccountId, ctx.offchainOrder.salt);
}
​
ctx.signer = ECDSA.recover(
_hashTypedDataV4(ctx.structHash), ctx.offchainOrder.v, ctx.offchainOrder.r, ctx.offchainOrder.s
);
​
if (ctx.signer != tradingAccount.owner) {
revert Errors.InvalidOrderSigner(ctx.signer, tradingAccount.owner);
}
​
// ...

As you can see, the function loops through all the orders and performs various verifications that, if not met, revert.

A user can easily block all the orders included with his like such :

  1. create an off-chain order (with nonce == 0) on a vivid market that has a lot of orders pending

  2. cancel his order by calling the OrderBranch::cancelAllOffchainOrders() function which will increment his nonce in storage

  3. his order is trying to be fulfilled among many others BUT since his tradingAccount.nonce is now equal to 1 because of previous step, the transaction will revert and no orders will be filled.

Impact

This can have a dramatic impact on traders who would want their orders to be executed as soon as possible to align with their market analysis.

Delaying the fulfillment could reduce the amount of profit they would have generated if their orders was filled correctly.

Tools Used

Manual review

Recommendations

The protocol partially implemented a way to mitigate the issue but did not implement it everywhere.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/SettlementBranch.sol#L289-L292

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

In case the order is invalid, rather than revert() the transaction should continue to loop over all the orders.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 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.

Give us feedback!