Submission Details
Severity:
Offchain Orders will bypass the Max Trade Positions per account check, hence a user can have unlimited number of trades thereby breaking the protocol's invariant
Summary
This report details a vulnerability in the protocol where offchain orders can bypass the maximum trade per account check. As a result, users can have an unlimited number of trades, thereby breaking the protocol's invariant. This issue allows users to create positions beyond the specified limit.
Vulnerability Details
The vulnerability arises because offchain orders, when settled on-chain, do not include the necessary invariant check for the maximum trade limit per account before trade execution. Specifically, the maximum trade limit check is omitted in the `fillOrder` function and `fillOffchainOrder`.
`fillOffchainOrder` can be used to create Limit orders. Hence this Vulnerability exits.
### Code Analysis
The `validatePositionsLimit` function is intended to ensure that a trading account does not exceed the maximum number of active positions. This function is called in the context of creating market orders but is missing from the `fillOrder` execution path.
/// @notice Validates if the trading account is under the configured positions limit.
/// @dev This function must be called when the trading account is going to open a new position. If called in a
/// context
/// of an already active market, the check may be misleading.
function validatePositionsLimit(Data storage self) internal view {
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
uint256 maxPositionsPerAccount = globalConfiguration.maxPositionsPerAccount;
uint256 activePositionsLength = self.activeMarketsIds.length();
if (activePositionsLength >= maxPositionsPerAccount) {
revert Errors.MaxPositionsPerAccountReached(self.id, activePositionsLength, maxPositionsPerAccount);
}
}
The check above can be found when validating an onchain marketorder
/// @notice Creates a market order for the given trading account and market ids.
/// @dev See {CreateMarketOrderParams}.
function createMarketOrder(CreateMarketOrderParams calldata params) external {
-------------------------------------------------------------------------
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExistingAccountAndVerifySender(params.tradingAccountId);
// find if account has active position in this market
@audit>> ctx.isMarketWithActivePosition = tradingAccount.isMarketWithActivePosition(params.marketId);
// if the account doesn't have an active position in this market then
// this trade is opening a new active position in a new market, hence
// revert if this new position would put the account over the maximum
// number of open positions
if (!ctx.isMarketWithActivePosition) {
@audit>> enforce limit for new positions >> tradingAccount.validatePositionsLimit();
}
function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
----------------------------------------------------------------------
@audit>> enforce limits for new positions/Validates trades first before excecution >>
// 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);
@audit>> Max trade per account not enforced, limits for new positions bypassed >>
// 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
);
}
In the `fillOrder` function, this validation is missing also, allowing users to bypass the limit with offchain limit orders.
function \_fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
// Other code omitted for brevity
@audit>> Max trade per account not enforced here also >>> // This check is missing and should be added here if 
// Other code omitted for brevity
}
Impact
The absence of the maximum trade limit check in the `fillOrder` function allows users to bypass the protocol's invariant, resulting in the following potential impacts:
1. **Unlimited Trades**: Users can create an unlimited number of trades, ignoring the maximum limit set by the protocol.
2. **Market Manipulation**: Users can exploit this vulnerability to manipulate the market by creating excessive positions.
Tools Used
Manual Code Review
Recommendations
To mitigate this vulnerability, it is crucial to include the `validatePositionsLimit` check in the `fillOrder` function, ensuring that the maximum trade limit is enforced consistently across all order types.
### Code Fix
Add the following validation in the `_fillOffchainOrder` function:
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);
}
++ bool isMarketWithActivePosition;
++ // find if account has active position in this market
++ isMarketWithActivePosition = tradingAccount.isMarketWithActivePosition(ctx.offchainOrder.marketId);
++ // if the account doesn't have an active position in this market then
++ // this trade is opening a new active position in a new market, hence
++ // revert if this new position would put the account over the maximum
++ // number of open positions
++ if (!isMarketWithActivePosition) {
++ tradingAccount.validatePositionsLimit();
++ }
// 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
);
}
}
Updates
Lead Judging Commences
inallhonesty 12 months ago
Submission Judgement Published Assigned finding tags:
`maxPositionsPerAccount` may be exceeded by combining onchain and offchain orders
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.