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

Users are able to bypass disabled market and still operate on them

greed

Summary

Zaros implements a way to disable a market and prevent users from opening a position in it. However, Zaros still allows users to reduce and close their existing positions since they can still be subject to liquidation.

The updatePerpMarketStatus() function is responsible for disabling a market.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/GlobalConfigurationBranch.sol#L570-L589

Vulnerability Details

The issue occurs due to a lack of checks in the createMarketOrder() function when an order is made to modify an existing position.

This function determines if the order being created will increase or decrease the position : if the position is initially a short of -10e18, increasing the position consists in making the position go even more negative, to -20e18 for example ; if the position is initially a long of 10e18, increasing the position consists in making the position go even more positive, to 20e18 for example.

The Position.isIncreasing() function is responsible for this check.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/Position.sol#L155-L172

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, not all the cases have been taken into account : a position can still satisfy the requirements by going from short to long, which will be considered as a decrease.

This basically leaves users the ability to interact with the market while they should not, based on the admins decision.

Impact

Disabling a market has not the expected effect on users that already have an existing position and allows them to interact with it (still making profits and losses).

Here is a coded PoC that demonstrates the issue :

It can be pasted in test\integration\perpetuals\perp-market-branch\getFundingRate\getFundingRate.t.sol

function testBypassMarketDisabled() external {
// give naruto some tokens
uint256 USER_STARTING_BALANCE = 100_000e18;
int128 USER_POS_SIZE_DELTA = 10e18;
deal({ token: address(usdz), to: users.naruto.account, give: USER_STARTING_BALANCE });
​
// naruto creates a trading account and deposits their tokens as collateral
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(USER_STARTING_BALANCE, address(usdz));
​
// naruto opens first position in BTC market
changePrank({ msgSender: users.naruto.account });
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, USER_POS_SIZE_DELTA);
​
SD59x18 equityUsd = perpsEngine.getAccountEquityUsd(tradingAccountId);
assertLt(equityUsd.intoInt256(), int256(USER_STARTING_BALANCE));
​
// protocol operator disables the BTC market
changePrank({ msgSender: users.owner.account });
perpsEngine.updatePerpMarketStatus({ marketId: BTC_USD_MARKET_ID, enable: false });
​
// naruto changes his position from LONG (10e18) to SHORT (-20e18)
changePrank({ msgSender: users.naruto.account });
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, -USER_POS_SIZE_DELTA * 3);
​
skip(5 days);
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE / 2);
​
// naruto position is still active and is now in profit
equityUsd = perpsEngine.getAccountEquityUsd(tradingAccountId);
assertGt(equityUsd.intoInt256(), int256(USER_STARTING_BALANCE));
}

Tools Used

Manual review

Recommendations

Add additionnal checks in the Position.isIncreasing() function to handle going from short to long and from long to short.

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!