Summary
Users should not be able to increase their position when the market or settlement is disabled. In such situations, users should only be able to decrease their positions. For example, if a user has a position size of 500, the new value of the position should be between 0 and 499. However, due to missing checks, it is currently possible to change the position to -1000, which breaks the protocol's invariants.
Vulnerability Details
The current implementation lacks necessary checks, allowing users to increase their positions from long to short and vice versa even when the market or settlement is disabled. This can lead to violations of the protocol's intended constraints.
Below is a test that demonstrates how the position can be increased from long to short, despite the market or settlement being disabled.
function test_WhenThePositionIsBeingIncreased()
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 * 3)
);
Impact
Users can bypass restrictions by increasing their positions from long to short or vice versa when the market or settlement is disabled. This breaks the protocol's invariants and could lead to financial instability and unexpected behavior in the trading system.
Tools Used
Manual review
Recommendations
Possible solution:
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);
+ return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0) || (self.size < 0 && self.size.add(sizeDelta) > 0 && self.size.add(sizeDelta).abs().gt(self.size.abs())) || (self.size > 0 && self.size.add(sizeDelta) < 0 && self.size.add(sizeDelta).abs().gt(self.size.abs()) ) ;
}
