From ddb602fba07de264491dd55b275badb2041df70f Mon Sep 17 00:00:00 2001 From: Julian R <56316686+julianmrodri@users.noreply.github.com> Date: Thu, 4 Jun 2020 02:58:51 -0300 Subject: [PATCH] feat: add wrapper function for low level calls --- contracts/mocks/AddressImpl.sol | 4 ++++ contracts/token/ERC20/SafeERC20.sol | 15 +++------------ contracts/token/ERC721/ERC721.sol | 19 +++---------------- contracts/utils/Address.sol | 27 +++++++++++++++++++++++++++ test/token/ERC20/SafeERC20.test.js | 2 +- test/token/ERC721/ERC721.test.js | 4 ++-- test/utils/Address.test.js | 4 ++++ 7 files changed, 44 insertions(+), 31 deletions(-) diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol index 0bcfcbb85fc..7bfcb7d8258 100644 --- a/contracts/mocks/AddressImpl.sol +++ b/contracts/mocks/AddressImpl.sol @@ -13,6 +13,10 @@ contract AddressImpl { Address.sendValue(receiver, amount); } + function functionCall(address target, bytes calldata data) external { + Address.functionCall(target, data); + } + // sendValue's tests require the contract to hold Ether receive () external payable { } } diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol index a815e3a3c4b..92142ae7ea7 100644 --- a/contracts/token/ERC20/SafeERC20.sol +++ b/contracts/token/ERC20/SafeERC20.sol @@ -56,19 +56,10 @@ library SafeERC20 { */ function _callOptionalReturn(IERC20 token, bytes memory data) private { // We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since - // we're implementing it ourselves. - - // A Solidity high level call has three parts: - // 1. The target address is checked to verify it contains contract code - // 2. The call itself is made, and success asserted - // 3. The return value is decoded, which in turn checks the size of the returned data. - // solhint-disable-next-line max-line-length - require(address(token).isContract(), "SafeERC20: call to non-contract"); - - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory returndata) = address(token).call(data); - require(success, "SafeERC20: low-level call failed"); + // we're implementing it ourselves. We use {Address.functionCall} to perform this call, which verifies that + // the target address contains contract code and also asserts for success in the low-level call. + bytes memory returndata = address(token).functionCall(data); if (returndata.length > 0) { // Return data is optional // solhint-disable-next-line max-line-length require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed"); diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 0e9653cef1e..1499aab4317 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -497,28 +497,15 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable if (!to.isContract()) { return true; } - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory returndata) = to.call(abi.encodeWithSelector( + bytes memory returndata = to.functionCall(abi.encodeWithSelector( IERC721Receiver(to).onERC721Received.selector, _msgSender(), from, tokenId, _data )); - if (!success) { - if (returndata.length > 0) { - // solhint-disable-next-line no-inline-assembly - assembly { - let returndata_size := mload(returndata) - revert(add(32, returndata), returndata_size) - } - } else { - revert("ERC721: transfer to non ERC721Receiver implementer"); - } - } else { - bytes4 retval = abi.decode(returndata, (bytes4)); - return (retval == _ERC721_RECEIVED); - } + bytes4 retval = abi.decode(returndata, (bytes4)); + return (retval == _ERC721_RECEIVED); } function _approve(address to, uint256 tokenId) private { diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index 2c54fe44968..1f326505cbc 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -57,4 +57,31 @@ library Address { (bool success, ) = recipient.call{ value: amount }(""); require(success, "Address: unable to send value, recipient may have reverted"); } + + /** + * @dev Replacement for Solidity's low-level `call`: performs a low-level + * call with `data` to the target address `target`. Returns the `returndata` + * provided by the low-level call. + * + * The call is not executed if the target address is not a contract. + */ + function functionCall(address target, bytes memory data) internal returns (bytes memory) { + require(isContract(target), "Address: call to non-contract"); + + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory returndata) = target.call(data); + if (!success) { + if (returndata.length > 0) { + // solhint-disable-next-line no-inline-assembly + assembly { + let returndata_size := mload(returndata) + revert(add(32, returndata), returndata_size) + } + } else { + revert("Address: low-level call failed"); + } + } else { + return returndata; + } + } } diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js index c50496ab7a0..11eac3100ef 100644 --- a/test/token/ERC20/SafeERC20.test.js +++ b/test/token/ERC20/SafeERC20.test.js @@ -15,7 +15,7 @@ describe('SafeERC20', function () { this.wrapper = await SafeERC20Wrapper.new(hasNoCode); }); - shouldRevertOnAllCalls('SafeERC20: call to non-contract'); + shouldRevertOnAllCalls('Address: call to non-contract'); }); describe('with token that returns false on all calls', function () { diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js index 10285322694..38a2ea90ca8 100644 --- a/test/token/ERC721/ERC721.test.js +++ b/test/token/ERC721/ERC721.test.js @@ -404,7 +404,7 @@ describe('ERC721', function () { const nonReceiver = this.token; await expectRevert( this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }), - 'ERC721: transfer to non ERC721Receiver implementer' + 'Address: low-level call failed' ); }); }); @@ -463,7 +463,7 @@ describe('ERC721', function () { const nonReceiver = this.token; await expectRevert( this.token.safeMint(nonReceiver.address, tokenId), - 'ERC721: transfer to non ERC721Receiver implementer' + 'Address: low-level call failed' ); }); }); diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index 1229302caa2..31e762b27ec 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -90,4 +90,8 @@ describe('Address', function () { }); }); }); + + describe('functionCall', function () { + // TODO + }); });