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

Offchain order getting filled for increased position also, without checking if settlement for such orders enabled

blutorque

Summary

Vulnerability Details

Order created via OrderBranch.createMarketOrder() has a settlement check which get triggered, when the position size is increasing either side, e.g., if

(1) position.size > 0 and sizeDelta > 0, then isIncreasing()==true
(2) position.size < 0 and sizeDelta < 0, then isIncreasing()==true

As explain in my other reports also, the protocol want to be at equilibrium keeping skew near 0. If position.size > 0 and sizeDelta < 0 or vice versa, the extras check won't be perform to such trade, orders can be freely created without reverting.

ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
​
// both markets and settlement can be disabled, however when this happens we want to:
// 1) allow open positions not subject to liquidation to decrease their size or close
// 2) prevent new positions from being opened & existing positions being increased
//
// the idea is to prevent a state where traders have open positions but are unable
// to reduce size or close even though they can still be liquidated; such a state
// would severly disadvantage traders
if (ctx.isIncreasing) { // @audit position getting increase, more market imbalance
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
​

And if the position size increases(either (1) or (2) occur), the settlement check would be triggered, assuming it's not active yet, the txn reverts. This is what we expected; https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L264-L266

// both markets and settlement can be disabled, however when this happens we want to:
// 1) allow open positions not subject to liquidation to decrease their size or close
// 2) prevent new positions from being opened & existing positions being increased

However, the issue lies with the offchain orders. The SettlementBranch.fillOffchainOrder() does not validate if the given position size isIncreasing() and, if so, whether the settlement is enabled. As a result, if settlement is not enabled(in which case txn should reverts) but user still wants more leverage, they can achieve this by placing offchain orders instead, which will bypass the settlement check on the increased position size.

Impact

Traders get more leverage to their increased position under condition where markets and settlement disabled.

Tools Used

Manual review

Recommendations

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
...snip...
​
if (ctx.signer != tradingAccount.owner) {
revert Errors.InvalidOrderSigner(ctx.signer, tradingAccount.owner);
}
+ bool isIncreasing = Position.isIncreasing(ctx.offchainOrder.tradingAccountId, ctx.offchainOrder.marketId, ctx.offchainOrder.sizeDelta);
+ if(isIncreasing) {
+ globalConfiguration.checkMarketIsEnabled(ctx.offchainOrder.marketId);
+ settlementConfiguration.checkIsSettlementEnabled();
+ }
​
...snip...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Disable market limitation can be bypassed

Support

FAQs

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

Give us feedback!