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

Incorrect inequality will cause an order to be in the pending state when it should not

arnie

Summary

Incorrect inequality will cause an order to be in the pending state when it should not

Vulnerability Detail

In MarketOrder.sol the function checkPendingOrder checks if an order is still pending, if it is, the function will revert

function checkPendingOrder(Data storage self) internal view {
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
uint128 marketOrderMinLifetime = globalConfiguration.marketOrderMinLifetime;
​
if (
self.timestamp != 0 && marketOrderMinLifetime != 0
&& block.timestamp - self.timestamp <= marketOrderMinLifetime
) {
revert Errors.MarketOrderStillPending(self.timestamp);
}
}
}

If we examine the snippet we see we load the marketOrderMinLifetime from the global configuration. marketOrderMinLifetime is the lowest amount of time until and order can be canceled, any time under this is considered to be a pending order still.

The problem occurs when we look at the if statement in the snippet

if (
self.timestamp != 0 && marketOrderMinLifetime != 0
&& block.timestamp - self.timestamp <= marketOrderMinLifetime
) {

as we can see if we specifically look at this block.timestamp - self.timestamp <= marketOrderMinLifetime, the self.timestamp is the time of the order creation, so when we do block.timestamp - self.timestamp we will get the amount of time the order has been pending for. but if we look at the inequality, it is inclusive when it shouldnt be. This means that even when the lifetime of an order is equal to the marketOrderMinLifetime, we will still revert.

This is not intended if we take a look at some of the comments in the protocol below

// 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();

as we can see

existed for the minimum order lifetime; pending orders can't be cancelled until they have existed for the minimum order lifetime

This means that an order should be able to be canceled if they have existed for the minimum order lifetime this is important.

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

function cancelMarketOrder(uint128 tradingAccountId) external {
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can cancel
// pending orders
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();
​
// reset pending order details
marketOrder.clear();
​
emit LogCancelMarketOrder(msg.sender, tradingAccountId);
}
}

the function above has a call to checkPendingOrder and even if the order has existed for the minimum order lifetime, it will still revert as i have explained above.

Impact

There are 2 impacts here

  1. an order will not be able to be canceled when it should be able to, this can lead to the order being filled the next block. The user should have been able to cancel the order but because of the incorrect inequality, he was not able to and his order filled.

  2. Creating a market order will also be slightly delayed because it also includes a call to checkPendingOrder, this can cause an order to be created the next block instead at a less favorable price because of the incorrect inequality.

Code Snippet

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/MarketOrder.sol#L55

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

Tool used

Manual Review

Recommendation

change <= to < in the checkPendingOrder function.

function checkPendingOrder(Data storage self) internal view {
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
uint128 marketOrderMinLifetime = globalConfiguration.marketOrderMinLifetime;
​
if (
self.timestamp != 0 && marketOrderMinLifetime != 0
&& block.timestamp - self.timestamp < marketOrderMinLifetime
) {
revert Errors.MarketOrderStillPending(self.timestamp);
}
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!