Consider simplified and gas-efficient alternatives to `Address.sol` to perform low level calls

[JacoboLansac]

Motivation

I recently audited a protocol that made an external call to a contract by using functionCall() from the Address.sol library. While auditing, I realized there is a significant gas saving that could affect all protocols using functionCall(), so I wanted to make a pull-request to address it.

Impact

• With this optimization, all calls to functionCallWithValue() that pass value=0 would save 100 gas.
• Notably, all contract calls using Address.functionCall() would benefit from this gas optimization as it calls functionCallWithValue() with value=0 hardcoded.

Details

The function functionCall() calls functionCallWithValue() passing uint256 value=0:

openzeppelin-contracts/contracts/utils/Address.sol

Lines 62 to 63 in e342516

	function functionCall(address target, bytes memory data) internal returns (bytes memory) {
	return functionCallWithValue(target, data, 0);

Then inside functionCallWithValue(), the library checks that address(this).balance is sufficient to cover for the value passed. Since functionCall() always calls with value=0, this will always be true, and the requirement will always pass:

openzeppelin-contracts/contracts/utils/Address.sol

Lines 75 to 80 in e342516

	function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
	if (address(this).balance < value) {
	revert Errors.InsufficientBalance(address(this).balance, value);
	}
	(bool success, bytes memory returndata) = target.call{value: value}(data);
	return verifyCallResultFromTarget(target, success, returndata);

However, reading an address.balance is a gas-expensive operation (100 gas according to https://www.evm.codes/), so it is inefficient to read the balance when the passed value is zero. A more gas-efficient implementation of functionCallWithValue() would only read the address.balance when the requested value is greater than zero:

    function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
-       if (address(this).balance < value) {
+       if ((value > 0) && (address(this).balance < value)) {
            revert Errors.InsufficientBalance(address(this).balance, value);
        }
        (bool success, bytes memory returndata) = target.call{value: value}(data);
        return verifyCallResultFromTarget(target, success, returndata);
    }

In the above, the EVM compiler is smart enough to know that the address.balance opcode is only used when the first if-condition is met. Therefore, any calls to functionCallWithValue() where value==0 would not read the balance unnecessarily.

[ernestognw]

Hi! Thanks for sharing

I generally agree with the analysis but I understand the main claim is that performing an address(this).balance is expensive, defined as being above 100 gas units. I think that's debatable. In practice, you can argue that contracts already using functionCallWithValue and upgrading to this new version will start consuming around 20 extra gas units (JUMPI plus some stack maneuvering). This would be the case for our AccessManager

Before going ahead with #5691, I think it's a better idea to reintroduce a library to perform calls. In retrospective the Address library is quite old and afaiu the intention was to abstract users the need to use .call directly. Now it's a completely different environment where everyone is comfortable using assembly calls. It may be better to provide optionality about whether to check value > 0 or address(this).balance in the library API rather than gas-golfing the current functions.

For reference, this may be a better option:

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

[ernestognw]

Some overlap with #5013

[JacoboLansac]

Hey @ernestognw thanks for taking it into consideration, and for the detailed answer. I was unaware of #5094, but if you think it is better to fix it there, you're probably right.

However, from what I see, a large number of protocols are used to import the Address.sol lib and use it blindly. A new library like in #5094 can be a good option for senior developers, but I'm afraid the Address.sol will keep being used. In that scenario, I would still argue it is an easy optimization that could impact many users overall over time.

But it is your call