Issue #1727: Improve revert reason when assuming interface compliance contracts. #1943

NoopurShinde · 2019-10-11T06:03:09Z

Solved issue Improve revert reason when assuming interface compliance #1727: Improve revert reason when assuming interface compliance contracts.

Fixes

Added the below code in ERC721.sol

  bytes memory payload = abi.encodeWithSignature(
   "onERC721Received(address,address,uint256,bytes)",
   msg.sender,
   from,
   tokenId,
   _data
   );
   
   (bool success, bytes memory returndata) = to.call(payload);

   if (!success) {
       if(returndata.length > 0){
           revert(string (returndata));
       }
       else
           revert("ERC721: to address does not implement ERC721Received interface");
   } else {
           return true;
          }

Added 2 dummy contracts in /contracts/mocks; one which does not implement ERC721Received and one which implements it but reverts as the contract rejects the transaction.
Updated the test cases in ERC721.behavior.js to test the code added in ERC721.sol

…-contracts

…#1727

NoopurShinde · 2019-10-17T06:34:00Z

Hi @nventuro @frangio

Any update on the PR?

frangio

Thank you @NoopurShinde! Sorry for the delay.

frangio · 2019-10-17T17:32:22Z

package.json

@@ -53,9 +53,9 @@
    "eslint-config-standard": "^11.0.0",
    "eslint-plugin-import": "^2.18.2",
    "eslint-plugin-mocha-no-only": "^1.1.0",
-    "eslint-plugin-node": "^5.2.1",
+    "eslint-plugin-node": "^10.0.0",


Can you please undo the changes in package.json? We prefer to have PRs that focus on separate issues.

frangio · 2019-10-17T17:33:26Z

contracts/token/ERC721/ERC721.sol

-        return (retval == _ERC721_RECEIVED);
+        bytes memory payload = abi.encodeWithSignature(
+            "onERC721Received(address,address,uint256,bytes)",
+            msg.sender,


Please use _msgSender() instead, for compatibility with Context. This is used for integration with the GSN.

Suggested change

msg.sender,

_msgSender(),

frangio · 2019-10-17T17:34:58Z

contracts/token/ERC721/ERC721.sol

+        bytes memory payload = abi.encodeWithSignature(
+            "onERC721Received(address,address,uint256,bytes)",


Please use abi.encodeWithSelector with IERC721Receiver(to).onERC721Received.selector as the first argument. This ensures that we don't mess up the string constant.

frangio · 2019-10-17T17:45:39Z

contracts/mocks/ERC721ReceiverNotImplementedMock.sol

+
+contract ERC721ReceiverNotImplementedMock{
+    bytes4 private _retval;
+    bool private _reverts;


Please remove these two variables since they're not used.

frangio · 2019-10-17T17:45:56Z

contracts/mocks/ERC721ReceiverRevertsMock.sol

+    function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data)
+        public returns (bytes4)
+    {
+        require(!_reverts, "ERC721ReceiverMock: Transaction rejected by receiver");


Suggested change

require(!_reverts, "ERC721ReceiverMock: Transaction rejected by receiver");

require(!_reverts, "ERC721ReceiverRevertsMock: Transaction rejected by receiver");

frangio · 2019-10-17T17:49:59Z

contracts/token/ERC721/ERC721.sol

+        (bool success, bytes memory returndata) = to.call(payload);
+        if (!success) {
+            if (returndata.length > 0){
+                revert(string(returndata));


From reading the Solidity documentation, it seems that both revert and require abi-encode their arguments as if they were a call to an Error(string) function.

If I understand correctly, this means that we're "wrapping" the revert reason twice, since returndata is already encoded here. I think we have to drop to assembly and use the revert opcode directly to avoid this re-encoding and to bubble up the value as-is.

Let me know if this was clear @NoopurShinde.

frangio · 2019-10-17T17:52:27Z

contracts/token/ERC721/ERC721.sol

+        if (!success) {
+            if (returndata.length > 0){
+                revert(string(returndata));
+            }else 
+                revert("ERC721: to address does not implement ERC721Received interface");
+        } else {return true;} 
+        // bytes4 retval = IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data);
+        // return (retval == _ERC721_RECEIVED);


Please tidy up the whitespace here to conform to our usual style and remove the commented out lines. 🙂

stale · 2019-11-02T03:43:52Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2019-11-21T12:07:11Z

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

stale · 2019-12-06T12:26:31Z

Hi folks!
This Pull Request is being closed as there was no response to the previous prompt. However, please leave a comment whenever you're ready to resume, so it can be reopened.
Thanks again!

…penZeppelin#1943

@frangio

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

@frangio

* adding mock contacts, test code * adding changes to ERC721.sol per @frangio's comments on original PR OpenZeppelin#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>

NoopurShinde and others added 8 commits October 11, 2019 14:03

Revert reason issue fixing

3e2ad2d

Revert reason issue fixing

3f38430

Merge branch 'master' of https://github.com/NoopurShinde/openzeppelin…

1ca3742

…-contracts

Improve revert reason when assuming interface compliance OpenZeppelin…

3d96ecf

…#1727

Merge branch 'master' into master

d210bae

Fixed npm lint issues

486f8db

Fixed npm lint issues

a625b8d

Fixed the ci/circleci: lint issue for solhint

a58cd80

frangio requested a review from nventuro October 15, 2019 16:32

frangio reviewed Oct 17, 2019

View reviewed changes

frangio removed the request for review from nventuro October 17, 2019 17:54

NoopurShinde and others added 2 commits October 18, 2019 09:08

Update package.json

4109cd6

Merge remote-tracking branch 'upstream/master'

af1372d

stale bot added the stale label Nov 2, 2019

Merge branch 'master' into master

06b34a0

stale bot removed the stale label Nov 6, 2019

frangio mentioned this pull request Nov 12, 2019

SafeERC20: forward revert reasons instead of "low-level call failed" #1805

Closed

stale bot added the stale label Nov 21, 2019

stale bot closed this Dec 6, 2019

frangio mentioned this pull request Dec 6, 2019

Improve revert reason when assuming interface compliance #1727

Closed

alant added a commit to alant/openzeppelin-solidity that referenced this pull request Dec 8, 2019

adding changes to ERC721.sol per @frangio's comments on original PR O…

bcb3125

…penZeppelin#1943

alant mentioned this pull request Dec 10, 2019

Fix/improve revert reason #1727 #2018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1727: Improve revert reason when assuming interface compliance contracts. #1943

Issue #1727: Improve revert reason when assuming interface compliance contracts. #1943

NoopurShinde commented Oct 11, 2019

NoopurShinde commented Oct 17, 2019

frangio left a comment

frangio Oct 17, 2019

frangio Oct 17, 2019

frangio Oct 17, 2019

frangio Oct 17, 2019

frangio Oct 17, 2019

frangio Oct 17, 2019

frangio Oct 17, 2019

stale bot commented Nov 2, 2019

stale bot commented Nov 21, 2019

stale bot commented Dec 6, 2019

		bytes memory payload = abi.encodeWithSignature(
		"onERC721Received(address,address,uint256,bytes)",

	require(!_reverts, "ERC721ReceiverMock: Transaction rejected by receiver");
	require(!_reverts, "ERC721ReceiverRevertsMock: Transaction rejected by receiver");

Issue #1727: Improve revert reason when assuming interface compliance contracts. #1943

Issue #1727: Improve revert reason when assuming interface compliance contracts. #1943

Conversation

NoopurShinde commented Oct 11, 2019

NoopurShinde commented Oct 17, 2019

frangio left a comment

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

frangio Oct 17, 2019

Choose a reason for hiding this comment

stale bot commented Nov 2, 2019

stale bot commented Nov 21, 2019

stale bot commented Dec 6, 2019