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

The function `fillOffchainOrders` can be DoS because in the loop it reverts instead of continue

draiakoo

Summary

The function fillOffchainOrders can be DoS because in the loop it reverts instead of continue

Vulnerability Details

The function to execute an order market offchain by the keepers is coded to be a batch of multiple signatures for the same market.

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
...
}

However, for each signature there are some checks to ensure a correct execution. These checks make the function to revert, so the problem is that if just a single signature does not comply with just 1 check it will revert the whole transaction and other signatures will not get executed either. The correct way to implement the checks is to just ignore the signature that does not comply the checks and execute the signatures that do.

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
// working data
FillOffchainOrders_Context memory ctx;
​
// fetch storage slot for perp market's offchain order config
SettlementConfiguration.Data storage settlementConfiguration =
SettlementConfiguration.load(marketId, SettlementConfiguration.OFFCHAIN_ORDERS_CONFIGURATION_ID);
​
// fetch storage slot for perp market
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
​
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
​
// verifies provided price data following the configured settlement strategy
// returning the bid and ask prices
(ctx.bidX18, ctx.askX18) =
settlementConfiguration.verifyOffchainPrice(priceData, globalConfiguration.maxVerificationDelay);
​
// iterate through off-chain orders; intentionally not caching
// length as reading from calldata is faster
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);
}
​
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 the offchain order has already been filled, revert.
// we store `ctx.hash`, and expect each order signed by the user to provide a unique salt so that filled
// orders can't be replayed regardless of the account's nonce.
if (tradingAccount.hasOffchainOrderBeenFilled[ctx.structHash]) {
revert Errors.OrderAlreadyFilled(ctx.offchainOrder.tradingAccountId, ctx.offchainOrder.salt);
}
​
// `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);
}
​
// cache the order side
ctx.isBuyOrder = ctx.offchainOrder.sizeDelta > 0;
​
// buy order -> match against the ask price
// sell order -> match against the bid price
ctx.indexPriceX18 = ctx.isBuyOrder ? ctx.askX18 : ctx.bidX18;
​
// verify the provided price data against the verifier and ensure it's valid, then get the mark price
// based on the returned index price.
ctx.fillPriceX18 = perpMarket.getMarkPrice(sd59x18(ctx.offchainOrder.sizeDelta), ctx.indexPriceX18);
​
// 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;
}
​
// account state updates start here
​
// increase the trading account nonce if the order's flag is true.
if (ctx.offchainOrder.shouldIncreaseNonce) {
unchecked {
tradingAccount.nonce++;
}
}
​
// mark the offchain order as filled.
// we store the struct hash to be marked as filled.
tradingAccount.hasOffchainOrderBeenFilled[ctx.structHash] = true;
​
// fill the offchain order.
_fillOrder(
ctx.offchainOrder.tradingAccountId,
marketId,
SettlementConfiguration.OFFCHAIN_ORDERS_CONFIGURATION_ID,
sd59x18(ctx.offchainOrder.sizeDelta),
ctx.fillPriceX18
);
}
}

As we can see in this implementation, there is a check that ignores the signature and execute the other ones. This check is for the valid 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;
}

This is the only check that si implemented good to not affect other signatures. However, there are a bunch of other checks that make the function to revert and a user can use it on purpose to make the transaction to revert and not execute other offchain signatures from the same market ID.
These checks are:

  1. If sizeDelta is 0, it reverts

  2. If marketId is different from the market passed by the user signature, it reverts. This check is not triggerable by the user because it is the responsability of the protocol to batch signatures from the same marketId.

  3. If the nonce passed by the signature does not match with the current position nonce, it reverts.

  4. If the signature has been executed previously and hasOffchainOrderBeenFilled is set to true, it reverts

  5. If the extracted signer of the signature is different from the position owner, it reverts.

  6. If the new position size is smaller than the minTradeSizeX18, it reverts

All of these checks will make the whole transaction to revert and almost all of them can be triggered by a malicious user. So the result will be that all other legit signatures for the same marketId will not be executed.

Impact

Medium

Tools Used

Manual review

Recommendations

For every check, instead of reverting, just continue with the next iteration of the loop to ignore the signature that fails the check. This way, the legit signatures will get executed properly.

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.