Improve revert reason when assuming interface compliance

[nventuro]

As discussed in this review comment, it'd be interesting to wrap calls to unknown contracts for which an interface is assumed in a low-level call that returns a custom revert reason on error, to better describe what went wrong.

[frangio]

A concrete example where whoever works on this should begin is this line in ERC721:

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/af55a843e3e2d9c941712788a3e0318988f2bd06/contracts/token/ERC721/ERC721.sol#L284

This line can revert if the receiver doesn't implement the onERC721Received function. It can also revert if it does implement it but something in the implementation like a require causes a revert.

This issue is about ensuring there is a nice error message in those cases.

The way in we can achieve it is by performing a low-level call which allows us to revert with a custom revert reason.

(bool success, bytes returndata) = to.call( ... );
if (!success) {
    // revert with custom reason (or use `returndata` if non-empty)
} else ...

[balajipachai]

@nventuro I have started to look into it

[balajipachai]

Hi @nventuro @frangio,
I have added below piece of 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) {
    revert("ERC721: to address does not implement ERC721Received interface");
} else {
          return true;
   }

While running the test cases it fails:

Any help on this will be much appreciated.

[NoopurShinde]

Hi Team,

I went through the specified issue. I have added the following code in ERC721.sol.

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

        (bool success, ) = to.call(payload);

        if (!success) {
            revert("ERC721: to address does not implement ERC721Received interface");
        } else {
                return true;
        }

I have created a dummy contract which does not implement the IERC721Receiver interface and have updated test cases for the same in ERC721.behavior.js

Please do let me know if this is the correct approach used or do I need to think of some other way to solve the issue.

[frangio]

That's pretty good @NoopurShinde!

It's also a possibility that the recipient does implement ERC721Received and has reverted to reject the transaction. In that case, we should bubble the revert reason up. We can account for this case by checking in the case of success == false whether the length of the returndata is greater than zero, and if so, bubbling that instead up of reverting with our own revert reason.

[NoopurShinde]

I have updated the code in ERC721.sol and ERC721Behavior.test.js as suggested by @frangio to solve the specified issues.
Whenever I run the test cases for ERC721, all the test cases are passed. But when I try to run the test suite, test cases result in failure. Is GSN causing the testcases to fail? I tried running test suite using truffle test and npm test. Please do let me know if this scenario is normal or is there something I am missing on.

[frangio]

Does someone want to pick up #1943 and continue the work?