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

Traders can still increase the position size when market and settlement are disabled and only need to deposit maintenance margin instead of initial margin

minhquanym

Summary

There is a missing case in the check of the function Position.isIncreasing(), allowing traders to bypass the restriction and increase their position size.

Basically, the issue is that the check does not consider the case where sizeDelta.abs() > self.size.abs() (trade size delta is larger than position size).

Vulnerability Details

In the Zaros protocol, the owner can enable or disable a market using the function updatePerpMarketStatus(). They can also enable or disable settlement using the function updateSettlementConfiguration(). When settlement is disabled, traders are not allowed to increase the position size. However, traders are still allowed to decrease the position size to avoid liquidation.

As a result, the contract needs to check if the position size is increasing when settling a trade through the Position.isIncreasing() function.

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);
}

However, this check lacks the case where the trade delta size is larger than the current position size.

Consider the scenario:

  1. Alice has a LONG position self.size = X (X > 0). She wants to increase this position to Y (Y > X > 0). But currently settlement is disabled, Alice should not be able to increase her LONG position.

  2. However, she can still do it by first decreasing her position with sizeDelta = -X - minTradeSize. Because self.size > 0 and sizeDelta < 0, this order is allowed. Now, the new position has self.size = -minTradeSize.

  3. Then she can increase her position with sizeDelta = minTradeSize + Y. Because self.size < 0 and sizeDelta > 0, this order is again allowed (bypassing the isIncreasing() check). Now, Alice has this new position with self.size = Y while settlement and the market are paused.

PoC (modified from test_WhenThePositionIsBeingDecreasedOrClosed())

function test_audit()
external
givenTheAccountIdExists
givenTheSenderIsAuthorized
whenTheSizeDeltaIsNotZero
givenThePerpMarketIsDisabled
{
// give naruto some tokens
uint256 marginValueUsdc = 100_000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
// naruto creates a trading account and deposits their tokens as collateral
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
// naruto opens first position in BTC market
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta
);
// protocol operator disables settlement for the BTC market
changePrank({ msgSender: users.owner.account });
SettlementConfiguration.DataStreamsStrategy memory marketOrderConfigurationData = SettlementConfiguration
.DataStreamsStrategy({ chainlinkVerifier: IVerifierProxy(mockChainlinkVerifier), streamId: BTC_USD_STREAM_ID });
​
SettlementConfiguration.Data memory marketOrderConfiguration = SettlementConfiguration.Data({
strategy: SettlementConfiguration.Strategy.DATA_STREAMS_DEFAULT,
isEnabled: false,
fee: DEFAULT_SETTLEMENT_FEE,
keeper: marketOrderKeepers[BTC_USD_MARKET_ID],
data: abi.encode(marketOrderConfigurationData)
});
perpsEngine.updateSettlementConfiguration(
BTC_USD_MARKET_ID, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID, marketOrderConfiguration
);
// naruto attempts to close their position
changePrank({ msgSender: users.naruto.account });
​
// naruto closes their opened leverage BTC position
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, -userPositionSizeDelta-userPositionSizeDelta-1
);
​
// naruto attempts to close their position
changePrank({ msgSender: users.naruto.account });
​
// naruto closes their opened leverage BTC position
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta+userPositionSizeDelta+userPositionSizeDelta
);
}

Impact

The check isIncreasing() is used in two places, each with different impacts:

  1. Order is created and filled. When settlement is paused, traders are still able to trade. This could be the case where a vulnerability is detected in the protocol and the owner disables it to fix the issue.

ctx.isIncreasing = Position.isIncreasing(tradingAccountId, marketId, sizeDeltaX18.intoInt256().toInt128());
​
// 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(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}

  1. Checking initial margin. Maintenance margin is used when isIncreasing = false. Because the maintenance margin is always less than the initial margin, traders could open/increase a position and only check for the maintenance margin instead of the initial margin. This will make the position liquidatable right after it is created, adding risk to the protocol.

ctx.shouldUseMaintenanceMargin = !ctx.isIncreasing && !ctx.oldPositionSizeX18.isZero();
// @audit could manipulate to check using maintenance margin instead of initial margin
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? requiredMaintenanceMarginUsdX18 : requiredInitialMarginUsdX18;
​
// reverts if the trader can't satisfy the appropriate margin requirement
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18,
tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18),
ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);

Tools Used

Manual Review

Recommendations

Consider adding a check to ensure that sizeDelta.abs() <= self.size.abs() if self.size != 0.

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.