Submission Details
Severity:
A malicious User can DOS all offchain orders making them unexecutable and leaving the protocol in an insolvent state. Also all offchain Trades can also be DOSed for honest parties that do not meet the fillorder requirements (no try and catch)
Summary
A malicious user can DOS all offchain orders, leaving the protocol insolvent. They exploit the lack of simulation for limit orders (limit/take profit/ stop limit) by creating zero-trade size orders. These are batched and executed once price criteria are met, causing repeated DOS attacks at low cost.
Vulnerability Details
A malicious user can exploit the current implementation of offchain orders by creating multiple offchain orders with a trade size of 0. This can lead to a Denial of Service (DoS) on the offchain order execution process, causing significant disruptions in the protocol's operation. The primary issues are:
1. **Zero-Size Orders**: Malicious users can create offchain orders with a trade size of 0, which can cause the entire batch of offchain orders to fail during execution.
2. **Validation are done**: Offchain orders are validated on chain at the point of execution before being included in the batch for onchain execution, making it possible for invalid orders to be processed (DOS for batched transactions).
Offchain order are batched together on chain before execution and executed once this gives room for exploit and DOSing the entire cached transactions.
@audit>> note >>> /// @notice Fills pending, eligible offchain offchain orders targeting the given market id.
/// @dev If a trading account id owner transfers their account to another address, all offchain orders will be
/// considered cancelled.
/// @param marketId The perp market id.
/// @param offchainOrders The array of signed custom orders.
/// @param priceData The price data of custom orders.
function fillOffchainOrders(
uint128 marketId,
@audit>> batched transactions >>> 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
@audit>> loop >>> for (uint256 i; i < offchainOrders.length; i++) {
ctx.offchainOrder = offchainOrders[i];
// enforce size > 0
@audit>> DOS attack possible revertion in loop during validation>>> if (ctx.offchainOrder.sizeDelta == 0) {
revert Errors.ZeroInput("offchainOrder.sizeDelta");
}
// load existing trading account; reverts for non-existent account
TradingAccount.Data storage tradingAccount =
@audit>> DOS attack possible revertion in loop during validation >>> TradingAccount.loadExisting(ctx.offchainOrder.tradingAccountId);
// enforce that keeper is filling the order for the correct marketId
@audit>> DOS attack possible revertion in loop during validation >>> 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.
@audit>> DOS attack possible revertion in loop during validation >>> 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.
@audit>> DOS attack possible revertion in loop during validation >>> 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.
@audit>> DOS attack possible revertion in loop during validation >>> 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
);
}
}
Market orders are simulated therefore there are little or no checks within fill market order because of the simulation function.
NOTE offchain orders are batched hence these DOS attack is possible.
For honest users -
Also if the DOS / revertion do occur. here below are the areas where a reversion can still be triggered WHEN FILLING THE ORDER.
/// @param tradingAccountId The trading account id.
/// @param marketId The perp market id.
/// @param settlementConfigurationId The perp market settlement configuration id.
/// @param sizeDeltaX18 The size delta of the order normalized to 18 decimals.
/// @param fillPriceX18 The fill price of the order normalized to 18 decimals.
function _fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
--------------------------------------------------------------
@audit>>> No try and catch--offchain can revert here/reversion in loop during execution(DOS)>>> perpMarket.checkTradeSize(sizeDeltaX18);
------------------------------------------------------------------------
@audit>>>No try and catch--offchain can revert here/reversion in loop during execution(DOS)>>> // reverts if the trader can't satisfy the appropriate margin requirement
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18,
tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18),
ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
}
----------------------------------------------------------------------
@audit>>> No try and catch--offchain can revert here/reversion in loop during execution(DOS)>>> // enforce open interest and skew limits for target market and calculate
// new open interest and new skew
(ctx.newOpenInterestX18, ctx.newSkewX18) =
perpMarket.checkOpenInterestLimits(sizeDeltaX18, ctx.oldPositionSizeX18, ctx.newPositionSizeX18);
Impact
1. **Delayed Execution**: Valid orders may be executed late, potentially filling orders at prices significantly different from the target price.
2. **Insolvency Risk**: The protocol could become insolvent if valid orders are consistently delayed or unexecuted.
Tools Used
- Manual Solidity code analysis
Recommendations
To mitigate this issue, implement an error emision and order skipping mechanism for batched offchain orders. Ensure that only orders meeting all criteria are met for onchain execution.
Consider nesting _fillOrder in a try and catch statement IN THE OFFCHAIN EXECUTION to prevent a complete DOS/revertion of all orders Knowing fully well that some orders are ok.
Explanation of Changes
-
Error Handling with
try-catch: The_fillOrderfunction call is now wrapped in atry-catchblock. If_fillOrderfails, anOrderErrorevent is emitted, and the loop continues to the next order. -
**Reverting Changes on Failure **: Reversion for ONCHAIN order reverts all actions when a reversion occurs during execution thus If
_fillOrderfails for offchain:The order is unmarked as filled.
The nonce is decremented if it was incremented earlier.
This ensures that a failure in _fillOrder does not revert the entire iteration and allows other orders to be processed.
/// @notice Fills pending, eligible offchain orders targeting the given market id.
/// @dev If a trading account id owner transfers their account to another address, all offchain orders will be
/// considered cancelled.
/// @param marketId The perp market id.
/// @param offchainOrders The array of signed custom orders.
/// @param priceData The price data of custom orders.
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");
-- }
// enforce size > 0
++ if (ctx.offchainOrder.sizeDelta == 0) {
++ emit OrderError(i, "Zero sizeDelta");
++ continue;
++ }
// load existing trading account; reverts for non-existent account
-- TradingAccount.Data storage tradingAccount =
-- TradingAccount.loadExisting(ctx.offchainOrder.tradingAccountId);
++ // load existing trading account; reverts for non-existent account
++ TradingAccount.Data storage tradingAccount;
++ try TradingAccount.loadExisting(ctx.offchainOrder.tradingAccountId) returns
++ (TradingAccount.Data storage account) {
++ tradingAccount = account;
++ } catch {
++ emit OrderError(i, "Non-existent trading account");
++ continue;
}
// enforce that keeper is filling the order for the correct marketId
if (marketId != ctx.offchainOrder.marketId) {
-- revert Errors.OrderMarketIdMismatch(marketId, ctx.offchainOrder.marketId);
++ emit OrderError(i, "Order marketId mismatch");
++ continue;
}
// 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);
++ emit OrderError(i, "Invalid signed nonce");
++ continue;
}
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);
++ emit OrderError(i, "Order already filled");
++ continue;
}
// `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);
++ emit OrderError(i, "Invalid order signer");
++ continue;
}
// 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) {
++ emit OrderError(i, "Invalid fill price");
continue;
}
// account state updates start here
// increase the trading account nonce if the order's flag is true or if the position is being reduced.
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;
-- _fillOrder(
-- ctx.offchainOrder.tradingAccountId,
-- marketId,
-- SettlementConfiguration.OFFCHAIN_ORDERS_CONFIGURATION_ID,
-- sd59x18(ctx.offchainOrder.sizeDelta),
-- ctx.fillPriceX18
-- );
// try to fill the offchain order and catch any potential errors
++ try this._fillOrder(
++ ctx.offchainOrder.tradingAccountId,
++ marketId,
++ SettlementConfiguration.OFFCHAIN_ORDERS_CONFIGURATION_ID,
++ sd59x18(ctx.offchainOrder.sizeDelta),
++ ctx.fillPriceX18
++ ) {
++ // if _fillOrder fails, emit an error and continue with the next order
++ } catch {
++ emit OrderError(i, "Failed to fill order");
++ // revert the marking of the order as filled since the actual fill failed
++ tradingAccount.hasOffchainOrderBeenFilled[ctx.structHash] = false;
++ // revert the nonce increment if it was done
++ if (ctx.offchainOrder.shouldIncreaseNonce) {
++ unchecked {
++ tradingAccount.nonce--;
++ }
}
}
}
}
Updates
Lead Judging Commences
inallhonesty about 1 year ago
Submission Judgement Published 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.
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.