Severity: Medium

Summary

Market orders are filled in two steps: trader creates an order and then keeper fills the order. The protocol peforms validations on the account when the order was created which may no longer hold by the time a keeper fills the order leading to a Time-Of-Check, Time-Of-Use (TOC, TOU) issue.

Vulnerability Details

Execution of market orders happens in two steps

1. Trader creates a market order using the OrderBranch::createMarketOrder function

2. Market keeper calls SettlementBranch::fillMarketOrder to fill the created order.

Three of the validations performed at the time of creation which might not hold:

1. The account active positions <= protocol limit

2. The account is not liquidatable

3. Account owner that created the market order is the owner when the order is filled

These invariants can change in-between step 1 and step 2:

1. Trader has positions in 4 markets and protocol limit is 5.

   • createMarketOrder allows for creating the position in 5th market.

   • Trader also creates an offchain order for a new market.

   • Trader creats a market order in 5th market.

   • The positions limit check will succeed as trader still has only 4 active positions.

   • The offchain order is filled before the market order.

   • The active positions become 5.

   • After the market order is filled it becomes 6.

2. The positions of the account might make losses because of price movements. The account might become liquidatable.

   • If the market order decreases the position size and the collateral balance satisfies margin requirements after the losses then the trade will succeed.

   • Protocol allowed for a trade of liquidatable account.

3. The owner of the trading account sells an account on an off-chain market. Before the NFT transfer transaction is included in the chain, the malicious owner creates a bad market order between the off-chian transaction and NFT transfer transaction. The market order is filled resulting in losses for the new owner.

Positions limit check in the createMarketOrder function: OrderBranch.sol#L285-L291

Liquidatable accounts check in the simulateTrade function:
OrderBranch.sol#L129-L136

The TradingAccountBranch::notifyOwner function does not clear the market order created by previous owner: TradingAccountBranch.sol#L405-L410

Definition of fillMarketOrder function: SettlementBranch.sol#L107-L114

Impact

• Traders can open position in 1 market more than the protocol limit

• Market order of a liquidatable account might be filled.

• The new owner might make losses after the transfer of the trading account NFT.

Tools Used

Manual Review

Recommendations

• Perform the validations at the time of filling the order in the SettlementBranch::_fillOrder function for the first two invariants.

• Update the notifyAccountTransfer function to clear any pending market order.