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 https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2018\#issuecomment-574381034

* 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>

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))

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);
+        }
     }
 
     /**

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