feelereth - Order placement in MultiInvoker is vulnerable to miner frontrunning

[sherlock-admin]

feelereth

high

Order placement in MultiInvoker is vulnerable to miner frontrunning

Summary

order placement in MultiInvoker emits an OrderPlaced event which could allow miners to frontrun and exploit upcoming order executions

Vulnerability Detail

When an order is placed, an OrderPlaced event is emitted with the order details (account, market, nonce, order) Link This allows miners to detect the upcoming order and frontrun its execution. For example:

1. User places a profitable order to buy assets when price drops below $1.
2. Miner detects the OrderPlaced event and sees the order details.
3. Before the next block is mined, the miner quickly executes the same trade themselves. 4.When the user's order executes, the opportunity is already gone.

Technically speaking Here is how it could be exploited:

1. User calls MultiInvoker.invoke() to place a order via _placeOrder().
2. _placeOrder() emits the OrderPlaced event before storing the order details in storage:
3. The transaction is broadcast to the network. 4. Miners see the OrderPlaced event in the pending transaction pool.

Malicious miners could construct and include a transaction in the next block to frontrun and exploit the upcoming order execution. For example, they could: • Place orders before the user's order executes to profit from the price impact • Cancel/modify the user's order before it executes • Liquidate the user's position if the order execution would make them undercollateralized

Impact

This is detrimental for users as miners can "steal" their orders and profits.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L408-L415

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L414

Tool used

Manual Review

Recommendation

the contract should avoid emitting events for order placement. Orders can be stored in a mapping without events. Replace the OrderPlaced event with an OrderFilled event when orders are actually executed

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

141345 commented:

d

panprog commented:

invalid because there is no funds loss in the frontrun and it doesn't depend on emit at all