feelereth - Updating positions before transferring fees means fees could be lost if transfer reverts.

[sherlock-admin2]

feelereth

high

Updating positions before transferring fees means fees could be lost if transfer reverts.

Summary

Updating positions before transferring fees means fees could be lost if transfer reverts. Should transfer first.

Vulnerability Detail

The main vulnerability is that _update() calls market.update() to update positions before withdrawing fees. If the fee withdrawal were to fail/revert for some reason, the fees would be lost.

Impact

If _withdraw() were to revert, the fee transfer would fail but the position update would still occur, losing fees.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L174 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L185 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L189

Tool used

Manual Review

Recommendation

call _withdraw() first before market.update

[sherlock-admin]

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

panprog commented:

invalid because transaction will revert entirely

n33k commented:

invalid, not convincing without PoC

darkart commented:

It happens in the same transacttion so it would revert

polarzero commented:

Medium. Downgrading to medium. Although the proposed fix does not seem to be sufficient, as it would introduce the opposite issue.