Zaros Part 1

Summary

By commitOrder then adjusting margin immediately after, a malicious trader gains control over whether his order is filled, allowing for risk-free trades and possible DOS of fillOffChainOrders.

Vulnerability Detail

During _fillOrder(), the transaction reverts if the account does not have enough margin:

A malicious trader can commit an order then immediately withdraw margin, putting his account below the margin requirement, if he does not want the order to be filled. When price or conditions moves in his favor, the trader can then depositMargin and allow the order to be filled.

Impact

This causes several issues:

1. The malicious trader gains granular control over whether his trades are fulfilled or not, at the price he desires. Also, unlike the typical flow of commitOrder -> wait for order to fill, through this method the trader can achieve a filled order faster. These advantages may earn the malicious trader more profits at the expense of the protocol/LPs who are the counterparty providing the liquidity.

2. Denial-of-Service(DOS) of fillOffChainOrders, which aims to fill a batch of orders. Other users' orders are not filled or delayed, resulting in loss of potential profits from trades. This can be performed repeatedly and at minimal cost to the attacker. Keepers also waste gas in the attempts to fill the orders.

Code Snippet

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/SettlementBranch.sol#L435
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/TradingAccountBranch.sol#L358

Tool used

Manual Review

Recommendation

During deposit/withdraw of margin cancel any pending orders by calling MarketOrder.load(ctx.tradingAccountId).clear(); similar to what is done during liquidations. This would prevent the trader from adjusting margin to prevent the order from filling.