Implement LowLevelCall library

[ernestognw]

Alternative to #5091
Fixes #5013
Requires #5189

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 51a3c50

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

I'd name that ...ReturnEmpty or ...NoReturn for consistency with ...ReturnBytes32

Suggested change

	function callRaw(address target, bytes memory data) internal returns (bool success) {
	function callReturnEmpty(address target, bytes memory data) internal returns (bool success) {

[Amxx]

Some comments:

• I don't think all the memory library is necessary here (for example the loadByte function). Agreeing on the low level call part should be our priority, and IMO we should not include additional things that are independant.

• We should thing of the "low level call" functions as an extension of the Address library, in terms of features AND naming. Ideally, in 6.0 I would move them all in the same file (IMO they are different ways, which different security guarantees of doing the same thing)

• Part of what makes low level call efficient is when you don't use abi.encode to prepare the call. This library doesn't support that. IMO its fine for now, but we should consider adding that later.

[Amxx]

Is there any case where the bubble is something other that the return data that we received from a call that was not successfull ?

I think that by definition of "bubble", it implies that its already an error that was reverted by an unsuccessfull call. Therefore, I'm would argue maybe the "low level call" function should include a boolean to select is yes/no we want to bubble any revert we get.

It could look like

    function callEmpty(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x00)
            if and(iszero(success), bubble) {
                let ptr := mload(0x40)
                returndatacopy(ptr, 0, returndatasize())
                revert(ptr, returndatasize())
            }
        }
    }

    function callScratch(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success, uint256 size, bytes32 x, bytes32 y) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x40)
            if and(iszero(success), bubble) {
                let ptr := mload(0x40)
                returndatacopy(ptr, 0, returndatasize())
                revert(ptr, returndatasize())
            }
            size := returndatasize()
            x := mload(0x00)
            y := mload(0x20)
        }
    }

    function callFull(address target, bytes memory data, uint256 value, bool bubble) internal returns (bool success, bytes memory returndata) {
        assembly ("memory-safe") {
            success := call(gas(), target, value, add(data, 0x20), mload(data), 0x00, 0x00)

            returndata := mload(0x40)
            returndatacopy(add(returndata, 0x20), 0, returndatasize())
            if and(iszero(success), bubble) {
                revert(add(returndata, 0x20), returndatasize())
            }
            mstore(returndata, returndatasize())
            mstore(0x40, add(add(returndata, 0x20), returndatasize()))
        }
    }

---

Note: The RETURNDATASIZE opcode is only 2 gas, and DUPX are 3 gas. So it turns out caching the returndatasize in the stack and dupping it is counterproductive !

EDIT: We decided to NOT do that bool bubble because the deicision to bubble or not might depend on the returndatasize we get.

[Amxx]

I updated the PR following our discussion of the day. I'm curious what the gas comparaison script tells us.

Note: the changes for SafeERC20 will be overwritten by the SafeERC20 PR.

[Amxx]

I did a lot of changes following yesterday's discussion. Only thing I havent done yet is polishing the contracts/utils/LowLevelCall.sol natspecs.

@ernestognw Let me know what you think !

[Amxx]

Gas number form CI

• Functions in Address.sol get cheaper by about 170gas (call) or 75gas (delegatecall)
• This impact other contract that use the Address library
  • AccessManager.execute
  • ERC20Multicall.multicall, BatchCaller.execute, TransientSlot.multicall ...
• SafeERC20 becomes more expensive by about ~200 gas. That is bad. It will be fixed by Refactor SafeERC20 to save gas #5771. We could
  • Merge that first, then do Refactor SafeERC20 to save gas #5771
  • Remove the SafeERC20 changes from this PR, then fo Refactor SafeERC20 to save gas #5771
  • do Refactor SafeERC20 to save gas #5771 first, then do this (without any change to SafeERC20.

[Amxx]

Current remarquable numbers:

• AccessManager.execute(address,bytes): -124 gas on average
• AccountERC7579.validateUserOp: -88 gas on average (-1833 in the worst case)
• Create2.deploy: -7500 gas on average
• ERC20Multicall: -530 gas (for batches of 2 calls)