Submission Details
Severity:
The function `fillOffchainOrders` does not check for the limit of active markets in a position
Summary
The function fillOffchainOrders does not check for the limit of active markets in a position
Vulnerability Details
When a user wants to create a market order, he has to initiate the process by calling the createMarketOrder function. In this function there is a check to ensure that the position does not exceed a limit for active markets.
// find if account has active position in this market
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) {
tradingAccount.validatePositionsLimit();
}
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);
}
}
It first checks if position has already a size in the market or is it new. If it is a new market, it ensures that the number of active markets in the position does not exceed a maximum.
However, when a user wants to create a market order offchain, he will sign the data to execute and the keeper will call the following function:
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);
}
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 notice here, this check to ensure a limit of active markets in the position is missing. So a user can bypass this limit just by executing the order markets offchain.
Impact
Medium
Tools Used
Manual review
Recommendations
Add this check to the fillOffchainOrders function:
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);
}
+ bool isMarketWithActivePosition = tradingAccount.isMarketWithActivePosition(ctx.offchainOrder.marketId);
+ if (!isMarketWithActivePosition) {
+ tradingAccount.validatePositionsLimit();
+ }
...
}
}
Updates
Lead Judging Commences
inallhonesty about 1 year 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.