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

Lack of slippage protection when creating Market orders

AuditTemple

Summary

Orders are created by accounts and executed at a later stage by off-chain keepers. The price at which the order is executed is based on the latest market conditions and current total positions in the market. This means that the order can be executed at an arbitrary fill price and the creator has no way to define min/max thresholds at which it will get executed, leading to losses.

Vulnerability Details

When an order is created through OrderBranch.createMarketOrder() the caller provides the following parameters:

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L242

struct CreateMarketOrderParams {
uint128 tradingAccountId;
uint128 marketId;
int128 sizeDelta;
}
​

Once an order is created subsequently an off-chain keeper picks it up and calls SettlementBranch.fillMarketOrder() to execute it, providing a price from an offline oracle:

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L138-L153

function fillMarketOrder(
uint128 tradingAccountId,
uint128 marketId,
bytes calldata priceData
)
external
onlyMarketOrderKeeper(marketId)
{
....
​
// verifies provided price data following the configured settlement strategy
// returning the bid and ask prices
(ctx.bidX18, ctx.askX18) =
settlementConfiguration.verifyOffchainPrice(priceData, globalConfiguration.maxVerificationDelay);
​
// cache the order's size delta
ctx.sizeDeltaX18 = sd59x18(marketOrder.sizeDelta);
​
....
// 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(ctx.sizeDeltaX18, ctx.indexPriceX18);
​
// perform the fill
_fillOrder(
tradingAccountId,
marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
ctx.sizeDeltaX18,
ctx.fillPriceX18
);
​
....
}
​

The off-chain price is used to calculate the final fillPrice in perpMarket.getMarkPrice() , which is based on the current conditions in the market - more specifically how the skew was changed by every trade before that and how the current trade changes it.

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/PerpMarket.sol#L107-L121

The problem with this is on a couple of levels:

  • the off-chain price might be disadvantageous to the user - it will be a valid price between the configured thresholds, but based on the positions an account has taken it might not be a price that is acceptable by the user

  • the execution of the fillMarketOrder might happen too late in time when market conditions have changed significantly compared to when the account was creating the order

  • the fillMarketOrder could get executed after a longer time at different market conditions due to unforeseen reasons, not in favour of the account that created the order.

Impact

MarketOrders can get executed at disadvantageous price levels than the accounts anticipated when orders were created.

Tools Used

Manual Review

Recommended Mitigation

Add min/maxFillPrice fields to the CreateMarketOrderParams struct, so that the account can guarantee the order will get executed at the price levels he expects.

Also consider adding a deadline parameter, that prevents stale transactions from executing.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

fillMarketOrder lacks slippage protection

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.