bigbick123456789000 - Use of `call()` Instead of `transfer()` in `MultiInvoker` 's `invoke` Function

[sherlock-admin4]

bigbick123456789000

medium

Use of call() Instead of transfer() in MultiInvoker 's invoke Function

Summary

The MultiInvoker contract employs the transfer() function when transferring Ether to recipients within its invoke() function. However, this approach can be problematic when the recipient is a smart contract with specific requirements for ETH transfers. To address this, the contract should utilize call() instead of transfer() to ensure compatibility with a wider range of smart contracts.

Vulnerability Detail

The invoke() function in the MultiInvoker contract aims to execute a series of actions defined in Invocation structs. These actions can involve interacting with other contracts, including transferring Ether. Currently, the function employs transfer() to send Ether back to the caller after executing the invocations. However, transfer() has limitations when transferring ETH to smart contracts:

• If the recipient contract does not have a payable fallback function, the transfer will fail.
• If the payable fallback function consumes more than 2300 gas units, the transfer will fail.
• If the payable fallback function consumes less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300, the transfer will fail. Here's a relevant code snippet from the invoke() function:

// Eth must not remain in this contract at rest
payable(msg.sender).transfer(address(this).balance);

Impact

The use of transfer() in the invoke() function can lead to failed ETH transfers, disrupting the intended functionality of the contract. This could result in funds being locked within the contract or transactions failing unexpectedly.

Code Snippet

MultiInvoker.sol#L163

Tool used

Manual Review

Recommendation

The MultiInvoker contract should replace transfer() with call() when transferring Ether back to the caller.

// Eth must not remain in this contract at rest
(bool success, ) = msg.sender.call{value: address(this).balance}("");
require(success, "Transfer failed");

[nevillehuang]

Invalid based on sherlock rules

10. Use of call vs transfer will be considered as a protocol design choice if there is no good reason why the call may consume more than 2300 gas without opcode repricings.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/equilibria-xyz/perennial-v2/pull/295

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.