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' ); }); });