Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 11 commits into from
14 changes: 14 additions & 0 deletions contracts/mocks/ERC721ReceiverNotImplementedMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
pragma solidity ^0.5.0;


contract ERC721ReceiverNotImplementedMock{
bytes4 private _retval;
bool private _reverts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas);

constructor (bytes4 retval, bool reverts) public {
_retval = retval;
_reverts = reverts;
}
}
23 changes: 23 additions & 0 deletions contracts/mocks/ERC721ReceiverRevertsMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
pragma solidity ^0.5.0;

import "../token/ERC721/IERC721Receiver.sol";

contract ERC721ReceiverRevertsMock is IERC721Receiver {
bytes4 private _retval;
bool private _reverts;

event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas);

constructor (bytes4 retval, bool reverts) public {
_retval = retval;
_reverts = reverts;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

emit Received(operator, from, tokenId, data, gasleft());
return _retval;
}
}
20 changes: 18 additions & 2 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,24 @@ contract ERC721 is Context, ERC165, IERC721 {
return true;
}

bytes4 retval = IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data);
return (retval == _ERC721_RECEIVED);
bytes memory payload = abi.encodeWithSignature(
"onERC721Received(address,address,uint256,bytes)",
Comment on lines +334 to +335
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

msg.sender,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
msg.sender,
_msgSender(),

from,
tokenId,
_data
);

//solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory returndata) = to.call(payload);
if (!success) {
if (returndata.length > 0){
revert(string(returndata));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}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);
Comment on lines +344 to +351
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down
31 changes: 21 additions & 10 deletions test/token/ERC721/ERC721.behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ const { shouldSupportInterfaces } = require('../../introspection/SupportsInterfa
const ERC721ReceiverMock = artifacts.require('ERC721ReceiverMock.sol');
const ERC721Mock = artifacts.require('ERC721Mock.sol');

// Required mock contracts
const ERC721NoReceiverMock = artifacts.require('./ERC721ReceiverNotImplementedMock.sol');
const ERC721ReceiverRevertMock = artifacts.require('./ERC721ReceiverRevertsMock.sol');

function shouldBehaveLikeERC721 (
creator,
minter,
Expand Down Expand Up @@ -296,25 +300,32 @@ function shouldBehaveLikeERC721 (

describe('to a receiver contract returning unexpected value', function () {
it('reverts', async function () {
const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
await expectRevert(
this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }),
'ERC721: transfer to non ERC721Receiver implementer'
this.token.safeTransferFrom(owner, noReceiverImplemented.address, tokenId, { from: owner }),
'ERC721: to address does not implement ERC721Received interface'
);
});
});

describe('to a receiver contract that throws', function () {
it('reverts', async function () {
const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
const invalidReceiver = await ERC721ReceiverRevertMock.new(RECEIVER_MAGIC_VALUE, true);
await expectRevert(
this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }),
'ERC721ReceiverMock: reverting'
'ERC721ReceiverMock: Transaction rejected by receiver'
);
});
});

describe('to a contract that does not implement the required function', function () {
it('reverts', async function () {
const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
await expectRevert(
this.token.safeTransferFrom(owner, noReceiverImplemented.address, tokenId, { from: owner }),
'ERC721: to address does not implement ERC721Received interface'
);
});
it('reverts', async function () {
const invalidReceiver = this.token;
await expectRevert.unspecified(
Expand Down Expand Up @@ -358,20 +369,20 @@ function shouldBehaveLikeERC721 (

context('to a receiver contract returning unexpected value', function () {
it('reverts', async function () {
const invalidReceiver = await ERC721ReceiverMock.new('0x42', false);
const noReceiverImplemented = await ERC721NoReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
await expectRevert(
this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
'ERC721: transfer to non ERC721Receiver implementer'
this.ERC721Mock.safeMint(noReceiverImplemented.address, tokenId),
'ERC721: to address does not implement ERC721Received interface'
);
});
});

context('to a receiver contract that throws', function () {
it('reverts', async function () {
const invalidReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true);
const invalidReceiver = await ERC721ReceiverRevertMock.new(RECEIVER_MAGIC_VALUE, true);
await expectRevert(
this.ERC721Mock.safeMint(invalidReceiver.address, tokenId),
'ERC721ReceiverMock: reverting'
'ERC721ReceiverMock: Transaction rejected by receiver'
);
});
});
Expand Down