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

Keepers can fill the orders before the user has a chance to cancel them

blutorque

Summary

Vulnerability Details

The Orderbranch.canceMarketOrder() function allows an active order to be cancel. It requires the order creator to wait for at least globalConfiguration.marketOrderMinLifetime seconds before canceling an active order. This delay has been effective in countering spam orders, especially in a busy market where it usually takes a bit longer for orders to be filled(as said here).

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

function cancelMarketOrder(uint128 tradingAccountId) external {
TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
​
// load trader's pending order; reverts if no pending order
MarketOrder.Data storage marketOrder = MarketOrder.loadExisting(tradingAccountId);
​
// reverts if a trader has a pending order and that pending order hasn't
// existed for the minimum order lifetime; pending orders can't be cancelled
// until they have existed for the minimum order lifetime
marketOrder.checkPendingOrder(); // @audit check min delay has passed
​
// reset pending order details
marketOrder.clear();
​
emit LogCancelMarketOrder(msg.sender, tradingAccountId);
}

The issue with this approach is, if a legitimate user wants to cancel their order after the delay has been passed, the order cannot be cancelled because it has already been filled or executed by keepers. This is due to the fact that keepers are not bound by this delay.

As a result, any unwanted trade that the user chooses to cancel may get executed before it becomes eligible for cancellation, leaving no room for the order to be cancelled, causing a potential damage to the user's position.

Impact

The likelihood is relatively higher, due to the facts its happening to every single order considering the keepers are bots.
The impact depends on the trade size, since there is no max cap it could be large enough to cause a potential fund loss to user position due to trade being not timely cancelled.

Tools Used

Manual review

Recommendations

Allow keepers to fill order only after the certain delay,

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

function fillMarketOrder(
uint128 tradingAccountId,
uint128 marketId,
bytes calldata priceData
)
external
onlyMarketOrderKeeper(marketId)
{
// working data
FillMarketOrder_Context memory ctx;
​
// fetch storage slot for perp market's market order config
SettlementConfiguration.Data storage settlementConfiguration =
SettlementConfiguration.load(marketId, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID);
​
// load trader's pending order; reverts if no pending order
MarketOrder.Data storage marketOrder = MarketOrder.loadExisting(tradingAccountId);
+ marketOrder.checkPendingOrder();
​
...snip...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!