From e57a570fd12d4e0d974428fb2f2a45c2a90497da Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Tue, 14 Jun 2022 01:15:44 +0800 Subject: [PATCH 1/6] check isContract only on success and no returndata --- contracts/mocks/crosschain/bridges.sol | 2 +- contracts/utils/Address.sol | 54 +++++++++++++++++--------- test/utils/Address.test.js | 6 +-- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/contracts/mocks/crosschain/bridges.sol b/contracts/mocks/crosschain/bridges.sol index 6a2b974702e..7df47bde282 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.verifyCallResult(target, success, returndata, "low-level call reverted"); _currentSender = previousSender; } diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index eae89956e0e..7fc08243f62 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 verifyCallResult(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 verifyCallResult(target, success, returndata, errorMessage); } /** @@ -186,10 +182,8 @@ 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 verifyCallResult(target, success, returndata, errorMessage); } /** @@ -198,6 +192,24 @@ library Address { * * _Available since v4.3._ */ + function verifyCallResult( + 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); + } + } + function verifyCallResult( bool success, bytes memory returndata, @@ -206,17 +218,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', ); }); }); From 1be67016a54a711687c17b234a73781ac26f98b9 Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Tue, 28 Jun 2022 01:18:20 +0800 Subject: [PATCH 2/6] verifyCallResult -> verifyCallResultFromTarget --- contracts/mocks/crosschain/bridges.sol | 2 +- contracts/utils/Address.sol | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/mocks/crosschain/bridges.sol b/contracts/mocks/crosschain/bridges.sol index 7df47bde282..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(target, 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 7fc08243f62..ee461d80f83 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -133,7 +133,7 @@ library Address { ) internal returns (bytes memory) { require(address(this).balance >= value, "Address: insufficient balance for call"); (bool success, bytes memory returndata) = target.call{value: value}(data); - return verifyCallResult(target, success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); } /** @@ -158,7 +158,7 @@ library Address { string memory errorMessage ) internal view returns (bytes memory) { (bool success, bytes memory returndata) = target.staticcall(data); - return verifyCallResult(target, success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); } /** @@ -183,7 +183,7 @@ library Address { string memory errorMessage ) internal returns (bytes memory) { (bool success, bytes memory returndata) = target.delegatecall(data); - return verifyCallResult(target, success, returndata, errorMessage); + return verifyCallResultFromTarget(target, success, returndata, errorMessage); } /** @@ -192,7 +192,7 @@ library Address { * * _Available since v4.3._ */ - function verifyCallResult( + function verifyCallResultFromTarget( address target, bool success, bytes memory returndata, From dbb9ea396f703fcaa6098fd03defcb5f43a8cd3f Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Tue, 28 Jun 2022 01:18:30 +0800 Subject: [PATCH 3/6] fix docs --- contracts/utils/Address.sol | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index ee461d80f83..b94ddbe6182 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -187,10 +187,10 @@ library Address { } /** - * @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 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.3._ + * _Available since v4.8._ */ function verifyCallResultFromTarget( address target, @@ -210,6 +210,12 @@ library Address { } } + /** + * @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._ + */ function verifyCallResult( bool success, bytes memory returndata, From 217520a266a4d318c455913f098e24295b355d56 Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Tue, 28 Jun 2022 01:25:50 +0800 Subject: [PATCH 4/6] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bb90621842..0ace3fa7f02 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`: perform `isContract` check only when the call was successful but returned no data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) ### Breaking changes From daf0b33e3599f368f0308432d9966431925b8604 Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Tue, 28 Jun 2022 01:30:19 +0800 Subject: [PATCH 5/6] fix linter --- contracts/utils/Address.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index b94ddbe6182..5c669091d8e 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -187,7 +187,7 @@ library Address { } /** - * @dev Tool to verify that a low level call to smart-contract was successful, and revert (either by bubbling + * @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._ From 80b8994d63dfc5d5eaa65023fc531a8019dbbb30 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 27 Jun 2022 16:52:09 -0300 Subject: [PATCH 6/6] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ace3fa7f02..e6581c7cc94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +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`: perform `isContract` check only when the call was successful but returned no data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) + * `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