feelereth - If collateral > 0, it pulls from msg.sender into this contract, then deposits to market. On withdraw, it goes directly to msg.sender. This asymmetry can lead to confusion.

[sherlock-admin]

feelereth

high

If collateral > 0, it pulls from msg.sender into this contract, then deposits to market. On withdraw, it goes directly to msg.sender. This asymmetry can lead to confusion.

Summary

There is an asymmetry in how collateral is transferred between the msg.sender and the market contract.

Vulnerability Detail

When depositing positive collateral:

1. Collateral is pulled from msg.sender into the MultiInvoker contract then deposited from MultiInvoker into the market: Link 1
2. But when withdrawing, Collateral is withdrawn directly from the market to msg.sender: Link 2 and Link 3 This can lead to confusion since deposits go through an intermediate step via MultiInvoker but withdraws do not. It also means there is no validation on the withdraw side that the market actually transferred the expected collateral to msg.sender.

Impact

This can allow the market to not properly transfer the withdrawn collateral, while MultiInvoker believes it was correctly handled.

Code Snippet

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

Tool used

Manual Review

Recommendation

Withdrawals should go through MultiInvoker similar to deposits:

1. Market withdraws collateral to MultiInvoker
2. MultiInvoker withdraws collateral to msg.sender

This adds symmetry on deposit and withdraw and allows MultiInvoker to validate the correct collateral amount is withdrawn from the market

[sherlock-admin]

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

panprog commented:

invalid because it works this way now (withdrawal via MultiInvoker) and moreover "can cause confusion" is not a valid issue by sherlock rules

n33k commented:

invalid, not convincing without PoC

darkart commented:

no impact and bad recommendation

polarzero commented:

Invalid. There is no evidence that this would cause confusion in the MultiInvoker contract.