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

User can increase size of their position on closed market.

baz1ka

Relevant GitHub Links

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

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

Summary

User should not increase but because of incorrect conditions when user changes sign of the position size this condition is skipped.

Impact

Impact is difficult to determine as there is no specification of why market can be disabled.

Proof of Concept

When user creates order that is new or increases existing position size it checks whether market is enabled. If market is not enabled transaction will revert (this OrderBranch.createMarketOrder() code part is the same as in SettlementBranch._fillOrder()):

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) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}

Let's take a look at what's inside Position.isIncreasing():

function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
Data storage self = Position.load(tradingAccountId, marketId);
​
// If position is being opened (size is 0) or if position is being increased (sizeDelta direction is same as
// position size)
// For a positive position, sizeDelta should be positive to increase, and for a negative position, sizeDelta
// should be negative to increase.
// This also implicitly covers the case where if it's a new position (size is 0), it's considered an increase.
@> return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}
}

This function returns true if there is no existing position (self.size == 0) or when position size is increased compared to existing position size, so if position size and sizeDelta have same sign. In case if signs are different this function would return false. This allows user to decrease (close) position (and condition from OrderBranch.createMarketOrder() so user can even close position for disabled market). But there is no condition that disallows user to flip position by changing position direction (from for example long to short). By doing so user can temporarily change position size, and then make same action and pass enough sizeDelta to flip position again and make new size bigger than initial. Here is steps example:
- user has existing buy position;
- owner closes market;
- user don't want to close position and want to increase this position size (maybe to get some profit from market price)
- user flips position by specifying sizeDelta.abs() > size (and size + sizeDelta > minimum position size), where sizeDelta sign is -;
- after this order is filled user creates similar order but with initial sizeDelta sign (+) and specifies such sizeDelta that is bigger than current position size (which is negative), big enough to flip position again and additional sizeDelta to increase initial position.

Recommended Mitigation Steps

If sign of initial size and newSize are different check whether market is enabled:

diff --git a/src/perpetuals/branches/OrderBranch.sol b/src/perpetuals/branches/OrderBranch.sol
index 5a0f075..a984213 100644
--- a/src/perpetuals/branches/OrderBranch.sol
+++ b/src/perpetuals/branches/OrderBranch.sol
@@ -261,6 +261,12 @@ contract OrderBranch {
// determine whether position is being increased or not
ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
+ Position.Data storage position = Position.load(params.tradingAccountId, params.marketId);
+
+ // fetch storage slot for account's potential existing position in this market
+ // int128 -> SD59x18
+ ctx.positionSizeX18 = sd59x18(position.size);
+
// 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
@@ -268,7 +274,11 @@ contract OrderBranch {
// 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) {
+
+ SD59x18 newPositionSize = ctx.positionSizeX18.add(ctx.sizeDeltaX18);
+
+ // If newPositionSize is 0 that means user closed position and we skip that
+ if (ctx.isIncreasing || !newPositionSize.isZero() && (ctx.positionSizeX18.gt(SD59x18_ZERO) != newPositionSize.gt(SD59x18_ZERO))) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
@@ -296,11 +306,6 @@ contract OrderBranch {
// enforce minimum trade size for this market
perpMarket.checkTradeSize(ctx.sizeDeltaX18);
- // fetch storage slot for account's potential existing position in this market
- Position.Data storage position = Position.load(params.tradingAccountId, params.marketId);
- // int128 -> SD59x18
- ctx.positionSizeX18 = sd59x18(position.size);
-
// enforce open interest and skew limits for target market
perpMarket.checkOpenInterestLimits(
ctx.sizeDeltaX18, ctx.positionSizeX18, ctx.positionSizeX18.add(ctx.sizeDeltaX18)
Updates

Lead Judging Commences

inallhonesty Lead Judge about 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.