feelereth - Vulnerability with the collateral transfers happening outside of the market.update() call in the _update() function

[sherlock-admin]

feelereth

high

Vulnerability with the collateral transfers happening outside of the market.update() call in the _update() function

Summary

The collateral transfers happen outside the market.update() call. This leaves room for issues if the market.update() reverts. It could lead to collateral getting stuck.

Vulnerability Detail

The key issue is that collateral is transferred to this contract before calling market.update(). If market.update() were to revert for any reason, the collateral would get stuck in this contract. A malicious actor could exploit this by:

1. Calling _update() with a large collateral deposit
2. Crafting the newMaker/newLong/newShort values to cause market.update() to revert
3. Collateral would get stuck in the contract and the user would lose their funds

Impact

Collateral would get stuck in the contract and the user would lose their funds

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L173-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

Move the collateral transfers inside the market.update() call. This ensures the collateral transfers happen atomically with the market update, preventing the stuck funds vulnerability

[sherlock-admin]

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

panprog commented:

invalid because transaction will revert entirely in this case (so transfer won't happen either, not just update reverts)

n33k commented:

invalid, not convincing without PoC

darkart commented:

Work as intended

polarzero commented:

Medium. Downgrading to medium as the conditions required to trigger the issue are not clearly demonstrated; the impact however would be critical.