feelereth - _executeOrder function is vulnerable to frontrunning attacks

[sherlock-admin]

feelereth

high

_executeOrder function is vulnerable to frontrunning attacks

Summary

The _executeOrder function can potentially be frontrun if the executor sees a pending profitable order

Vulnerability Detail

This is possible because the reentrancy guard is only checking for calls at the start of withdraw(). The _ensureApprove() call happens after the state change of burning shares, so it is vulnerable.

This Link reads the pending position, executes the order against it, and commits the updated position. The vulnerability comes from the fact that between reading the pending position and committing it, another transaction could come in and update the pending position. For example:

1. _executeOrder reads currentPosition with 100 DAI in long
2. Attacker frontruns and updates pending position to long 200 DAI
3. _executeOrder executes the order against the old 100 DAI position
4. _executeOrder commits 200 DAI long instead of the expected position This could allow the attacker to profit by manipulating the pending position before _executeOrder commits.

Impact

The attacker profit by manipulating the pending position before _executeOrder commits

Code Snippet

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

Tool used

Manual Review

Recommendation

• Locking the pending position before reading
• Committing the updated position atomically

[sherlock-admin]

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

141345 commented:

d

panprog commented:

invalid because scenario doesn't make sense and mentions internal function which can't be frontrun