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

A malicious user could create a DoS for `fillOffchainOrders` and no Order can be filled

0xaman

Summary

The fillOffchainOrders function is used to submit orders on-chain, which were signed by their owners off-chain in a batch by a Keeper. However, malicious users can create a DoS attack by transferring their TradingAccount or updating their nonce.

Vulnerability Details

The Normal flow for fillOffChainOrder is the users sign their orders off-chain . The keeper will pick them and submit them in a batch to on-chain.

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
...
for (uint256 i; i < offchainOrders.length; i++) {
ctx.offchainOrder = offchainOrders[i];
​
// enforce size > 0
if (ctx.offchainOrder.sizeDelta == 0) {
revert Errors.ZeroInput("offchainOrder.sizeDelta");
}
​
// load existing trading account; reverts for non-existent account
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExisting(ctx.offchainOrder.tradingAccountId);
​
// enforce that keeper is filling the order for the correct marketId
if (marketId != ctx.offchainOrder.marketId) {
revert Errors.OrderMarketIdMismatch(marketId, ctx.offchainOrder.marketId);
}
​
// First we check if the nonce is valid, as a first measure to protect from replay attacks, according to
// the offchain order's type (each type may have its own business logic).
// e.g TP/SL must increase the nonce in order to prevent older limit orders from being filled.
// NOTE: Since the nonce isn't always increased, we also need to store the typed data hash containing the
// 256-bit salt value to fully prevent replay attacks.
if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
@> revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
}
....
// `ecrecover`s the order signer.
ctx.signer = ECDSA.recover(
_hashTypedDataV4(ctx.structHash), ctx.offchainOrder.v, ctx.offchainOrder.r, ctx.offchainOrder.s
);
​
// ensure the signer is the owner of the trading account, otherwise revert.
// NOTE: If an account's owner transfers to another address, this will fail. Therefore, clients must
// cancel all users offchain orders in that scenario.
if (ctx.signer != tradingAccount.owner) {
@> revert Errors.InvalidOrderSigner(ctx.signer, tradingAccount.owner); // @audit-issue
}
}
}

This function would cancel all the OffChain order :

2024-07-zaros/src/perpetuals/branches/OrderBranch.sol:365
365: function cancelAllOffchainOrders(uint128 tradingAccountId) external {
366: // load existing trading account; reverts for non-existent account
367: // enforces `msg.sender == owner` so only account owner can cancel
368: // pendin orders.
369: TradingAccount.Data storage tradingAccount =
370: TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
371:
372: // bump the nonce value, which will invalidate all offchain orders signed with the previous nonce
373: unchecked {
374: tradingAccount.nonce++;
375: }
376:
377: emit LogCancelAllOffchainOrders(msg.sender, tradingAccountId, tradingAccount.nonce);
378: }

In the code above, if the owner of the TradingAccount does not match the signer or if the nonce has already been used, the function will revert. A malicious user can exploit this to create a permanent DoS attack by either transferring their account or resetting the nonce. Since transaction costs on Arbitrum are low, this attack is feasible. Additionally, this should not be confused with front-running, as front-running cannot occur on Arbitrum.

POC:

  1. Alice wants to create a DoS for fillOffChainOrders.

  2. Alice signs the OffChainOrder, let's suppose at block 1.

  3. Alice either calls transferFrom on the ERC721 contract to transfer the account or calls cancelAllOffChainOrder to invalidate the nonce.

  4. The offchain orders are still to be picked by the Keeper.

  5. At block 3, the Keeper picks the offchain order batch and tries to submit it on-chain.

  6. The transaction would revert with either InvalidOrderSigner due to the token transfer or InvalidSignedNonce due to resetting the nonce.

Impact

The malicious user could create a permanent DoS by signing an off-chain order and then resetting their nonce or transferring the account ownership.

Tools Used

Manual Review

Recommendations

If the nonce or ownership is invalid, avoid reverting the transaction. Instead, emit a log to notify the off-chain system and proceed with execution.

Updates

Lead Judging Commences

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