From 6241352d8aba6c1209ba5e1787bf34902115c749 Mon Sep 17 00:00:00 2001 From: ronhuafeng Date: Fri, 9 Sep 2022 17:09:16 +0100 Subject: [PATCH] Optimize Address.functionCall removing redundant isContract check (#3469) Co-authored-by: Francisco --- CHANGELOG.md | 1 + contracts/mocks/crosschain/bridges.sol | 2 +- contracts/utils/Address.sol | 64 +++++++++++++++++--------- test/utils/Address.test.js | 6 +-- 4 files changed, 48 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ee50f71f81..34a569669b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ * `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350)) * `Initializable`: refactored implementation of modifiers for easier understanding. ([#3450](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3450)) * `Proxies`: remove runtime check of ERC1967 storage slots. ([#3455](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3455)) + * `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) ### Breaking changes diff --git a/contracts/mocks/crosschain/bridges.sol b/contracts/mocks/crosschain/bridges.sol index 6a2b974702e..1339f4371c0 100644 --- a/contracts/mocks/crosschain/bridges.sol +++ b/contracts/mocks/crosschain/bridges.sol @@ -22,7 +22,7 @@ abstract contract BaseRelayMock { _currentSender = sender; (bool success, bytes memory returndata) = target.call(data); - Address.verifyCallResult(success, returndata, "low-level call reverted"); + Address.verifyCallResultFromTarget(target, success, returndata, "low-level call reverted"); _currentSender = previousSender; } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index d1e94832f72..544c357e593 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -132,10 +132,8 @@ library Address { string memory errorMessage ) internal returns (bytes memory) { require(address(this).balance >= value, "Address: insufficient balance for call"); - require(isContract(target), "Address: call to non-contract"); - (bool success, bytes memory returndata) = target.call{value: value}(data); - return verifyCallResult(success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); } /** @@ -159,10 +157,8 @@ library Address { bytes memory data, string memory errorMessage ) internal view returns (bytes memory) { - require(isContract(target), "Address: static call to non-contract"); - (bool success, bytes memory returndata) = target.staticcall(data); - return verifyCallResult(success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); } /** @@ -186,15 +182,37 @@ library Address { bytes memory data, string memory errorMessage ) internal returns (bytes memory) { - require(isContract(target), "Address: delegate call to non-contract"); - (bool success, bytes memory returndata) = target.delegatecall(data); - return verifyCallResult(success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); + } + + /** + * @dev Tool to verify that a low level call to smart-contract was successful, and revert (either by bubbling + * the revert reason or using the provided one) in case of unsuccessful call or if target was not a contract. + * + * _Available since v4.8._ + */ + function verifyCallResultFromTarget( + address target, + bool success, + bytes memory returndata, + string memory errorMessage + ) internal view returns (bytes memory) { + if (success) { + if (returndata.length == 0) { + // only check isContract if the call was successful and the return data is empty + // otherwise we already know that it was a contract + require(isContract(target), "Address: call to non-contract"); + } + return returndata; + } else { + _revert(returndata, errorMessage); + } } /** - * @dev Tool to verifies that a low level call was successful, and revert if it wasn't, either by bubbling the - * revert reason using the provided one. + * @dev Tool to verify that a low level call was successful, and revert if it wasn't, either by bubbling the + * revert reason or using the provided one. * * _Available since v4.3._ */ @@ -206,17 +224,21 @@ library Address { if (success) { return returndata; } else { - // Look for revert reason and bubble it up if present - if (returndata.length > 0) { - // The easiest way to bubble the revert reason is using memory via assembly - /// @solidity memory-safe-assembly - assembly { - let returndata_size := mload(returndata) - revert(add(32, returndata), returndata_size) - } - } else { - revert(errorMessage); + _revert(returndata, errorMessage); + } + } + + function _revert(bytes memory returndata, string memory errorMessage) private pure { + // Look for revert reason and bubble it up if present + if (returndata.length > 0) { + // The easiest way to bubble the revert reason is using memory via assembly + /// @solidity memory-safe-assembly + assembly { + let returndata_size := mload(returndata) + revert(add(32, returndata), returndata_size) } + } else { + revert(errorMessage); } } } diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index 6cd202c545a..1bdfad48366 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -143,7 +143,7 @@ contract('Address', function (accounts) { }, []); await expectRevert( - this.mock.functionCall(this.contractRecipient.address, abiEncodedCall, { gas: '100000' }), + this.mock.functionCall(this.contractRecipient.address, abiEncodedCall, { gas: '120000' }), 'Address: low-level call failed', ); }); @@ -329,7 +329,7 @@ contract('Address', function (accounts) { }, []); await expectRevert( this.mock.functionStaticCall(recipient, abiEncodedCall), - 'Address: static call to non-contract', + 'Address: call to non-contract', ); }); }); @@ -375,7 +375,7 @@ contract('Address', function (accounts) { }, []); await expectRevert( this.mock.functionDelegateCall(recipient, abiEncodedCall), - 'Address: delegate call to non-contract', + 'Address: call to non-contract', ); }); });