Fix/improve revert reason #1727 (#2018)

* 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 <frangio.1@gmail.com> * 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 <frangio.1@gmail.com> * Update contracts/token/ERC721/ERC721.sol Co-Authored-By: Francisco Giordano <frangio.1@gmail.com> * 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 #2018 * Add changelog entry * Fix tests * Make tests more clear Co-authored-by: Francisco Giordano <frangio.1@gmail.com> Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
OpenZeppelin · Jan 23, 2020 · 7d7cbca · 7d7cbca
1 parent e493fb3
commit 7d7cbca
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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,19 +307,20 @@ 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'
             );
           });
         });
 
         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,19 +370,20 @@ 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'
             );
           });
         });
 
         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'
             );
           });
         });