Summary
The current logic for determining whether a position is being increased is flawed. It allows users to increase their position size in other direction by specifying an absolute value of sizeDelta that is more than their current position size.
Vulnerability Details
If the perpMarketis not enabled for trading or it's settlementConfigurationis not enabled, then the checks are present in code to ensure that users can only decrease their size or close the position. Due to this, new positions in that market can't be opened or existing position can't be increased.
However, the checks present is not sufficient and user can still trade when markets and settlement can be disabled. Due to insufficient check, user can convert their long position to short or their short position to long. This is not desirable since users are only allowed to decrease their size or close the position. The users can change their long position to short by entering sizeDelta (in absolute terms) more than abs(position.size) + minTradeSize. This is not desirable because trading activity will still be enabled due to bypassing of checks by users.
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L262
ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
if (ctx.isIncreasing) {
globalConfiguration.checkMarketIsEnabled(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L367-L380
ctx.isIncreasing = Position.isIncreasing(tradingAccountId, marketId, sizeDeltaX18.intoInt256().toInt128());
if (ctx.isIncreasing) {
globalConfiguration.checkMarketIsEnabled(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/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);
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}
Proof of concept:
Below is the modified test named test_WhenThePositionIsBeingDecreasedOrClosedin createMarketOrder.t.sol. Line 39in below test is modified. In the original test, the assumption was that if market is disabled, only the position can be decreased or closed. However, we increased user's long position to short instead of closing it.
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/test/integration/perpetuals/order-branch/createMarketOrder/createMarketOrder.t.sol#L182-L222
function test_WhenThePositionIsBeingDecreasedOrClosed()
external
givenTheAccountIdExists
givenTheSenderIsAuthorized
whenTheSizeDeltaIsNotZero
givenThePerpMarketIsDisabled
{
uint256 marginValueUsdc = 100_000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta
);
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
);
changePrank({ msgSender: users.naruto.account });
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, -userPositionSizeDelta - 5e18
);
}
Impact
The user can still continue their trading activities even though the marketand settlementConfigurationis disabled by admin.
Tools Used
Manual Review
Recommendations
The case where position is not increasing, another check should be added to ensure that abs(sizeDelta)should be less than or equal to abs(positionSize).
ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
if (ctx.isIncreasing && abs(sizeDelta) < abs(position.size)) {
globalConfiguration.checkMarketIsEnabled(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
