From 7d7cbcad14aa17b0202f67e7d8d619f1687c94a6 Mon Sep 17 00:00:00 2001 From: "Hao (Alan) Tang" Date: Thu, 23 Jan 2020 11:33:09 -0800 Subject: [PATCH] Fix/improve revert reason #1727 (#2018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * adding mock contacts, test code * adding changes to ERC721.sol per @frangio's comments on original PR #1943 * fix solhint warnings * Update contracts/token/ERC721/ERC721.sol Co-Authored-By: Francisco Giordano * same revert wording per @frangio's review suggestion * per @frangio's feedback, changing the inline assembly to accomplish: we want to ignore the first 4 bytes of content, so we should read the length and decrease it by 4, then take the memory location and add 4 to it, then store the new length at the new memory location, then that is the new byte array that we want. * change revert msg assembly per PR comment by @frangio * unify revert msg in test code * fix some failed tests, wording change * Update contracts/token/ERC721/ERC721.sol Co-Authored-By: Francisco Giordano * Update contracts/token/ERC721/ERC721.sol Co-Authored-By: Francisco Giordano * fix test case, revert without reason * fix 'ERC721ReceiverRevertsMock: Transaction rejected by receiver' * style change per review by @frangio * fix revert reason forwarding * remove duplicate contracts/mocks/ERC721ReceiverRevertsMock.sol per review https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2018\#issuecomment-574381034 * Add changelog entry * Fix tests * Make tests more clear Co-authored-by: Francisco Giordano Co-authored-by: Nicolás Venturo --- CHANGELOG.md | 1 + contracts/token/ERC721/ERC721.sol | 25 ++++++++++++++++++++++--- test/token/ERC721/ERC721.behavior.js | 24 +++++++++++++----------- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 373e93fe5..ece6c2fbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Improvements * `ERC777`: `_burn` is now internal, providing more flexibility and making it easier to create tokens that deflate. ([#1908](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1908)) * `ReentrancyGuard`: greatly improved gas efficiency by using the net gas metering mechanism introduced in the Istanbul hardfork. ([#1992](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1992), [#1996](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1996)) + * `ERC721`: improved revert reason when transferring tokens to a non-recipient contract. ([#2018](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2018)) ### Breaking changes * `ERC165Checker` now requires a minimum Solidity compiler version of 0.5.10. ([#1829](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1829)) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 57c96e3a5..67742b842 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -330,9 +330,28 @@ contract ERC721 is Context, ERC165, IERC721 { if (!to.isContract()) { return true; } - - bytes4 retval = IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data); - return (retval == _ERC721_RECEIVED); + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory returndata) = to.call(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); + } } /** diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 8d8b8a464..bca0fc924 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -4,8 +4,8 @@ const { expect } = require('chai'); const { ZERO_ADDRESS } = constants; const { shouldSupportInterfaces } = require('../../introspection/SupportsInterface.behavior'); -const ERC721ReceiverMock = contract.fromArtifact('ERC721ReceiverMock'); const ERC721Mock = contract.fromArtifact('ERC721Mock'); +const ERC721ReceiverMock = contract.fromArtifact('ERC721ReceiverMock'); function shouldBehaveLikeERC721 ( creator, @@ -307,9 +307,9 @@ function shouldBehaveLikeERC721 ( describe('to a receiver contract that throws', function () { it('reverts', async function () { - const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); await expectRevert( - this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }), + this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), 'ERC721ReceiverMock: reverting' ); }); @@ -317,9 +317,10 @@ function shouldBehaveLikeERC721 ( describe('to a contract that does not implement the required function', function () { it('reverts', async function () { - const invalidReceiver = this.token; - await expectRevert.unspecified( - this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }) + const nonReceiver = this.token; + await expectRevert( + this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }), + 'ERC721: transfer to non ERC721Receiver implementer' ); }); }); @@ -369,9 +370,9 @@ function shouldBehaveLikeERC721 ( context('to a receiver contract that throws', function () { it('reverts', async function () { - const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); await expectRevert( - this.ERC721Mock.safeMint(invalidReceiver.address, tokenId), + this.ERC721Mock.safeMint(revertingReceiver.address, tokenId), 'ERC721ReceiverMock: reverting' ); }); @@ -379,9 +380,10 @@ function shouldBehaveLikeERC721 ( context('to a contract that does not implement the required function', function () { it('reverts', async function () { - const invalidReceiver = this.token; - await expectRevert.unspecified( - this.ERC721Mock.safeMint(invalidReceiver.address, tokenId) + const nonReceiver = this.token; + await expectRevert( + this.ERC721Mock.safeMint(nonReceiver.address, tokenId), + 'ERC721: transfer to non ERC721Receiver implementer' ); }); });