diff --git a/contracts/access/TimelockController.sol b/contracts/access/TimelockController.sol index 5e70362f3..cba9b6895 100644 --- a/contracts/access/TimelockController.sol +++ b/contracts/access/TimelockController.sol @@ -20,7 +20,6 @@ import "./AccessControl.sol"; * _Available since v3.3._ */ contract TimelockController is AccessControl { - bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE"); bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE"); bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); diff --git a/contracts/introspection/ERC165.sol b/contracts/introspection/ERC165.sol index cb120911a..7c4d3639f 100644 --- a/contracts/introspection/ERC165.sol +++ b/contracts/introspection/ERC165.sol @@ -11,11 +11,6 @@ import "./IERC165.sol"; * their support of an interface. */ abstract contract ERC165 is IERC165 { - /* - * bytes4(keccak256('supportsInterface(bytes4)')) == 0x01ffc9a7 - */ - bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7; - /** * @dev Mapping of interface ids to whether or not it's supported. */ @@ -24,7 +19,7 @@ abstract contract ERC165 is IERC165 { constructor () { // Derived contracts need only register support for their own interfaces, // we register support for ERC165 itself here - _registerInterface(_INTERFACE_ID_ERC165); + _registerInterface(type(IERC165).interfaceId); } /** diff --git a/contracts/introspection/ERC165Checker.sol b/contracts/introspection/ERC165Checker.sol index cb37b69f0..4147f8cf9 100644 --- a/contracts/introspection/ERC165Checker.sol +++ b/contracts/introspection/ERC165Checker.sol @@ -2,6 +2,8 @@ pragma solidity ^0.8.0; +import "./IERC165.sol"; + /** * @dev Library used to query support of an interface declared via {IERC165}. * @@ -13,18 +15,13 @@ library ERC165Checker { // As per the EIP-165 spec, no interface should ever match 0xffffffff bytes4 private constant _INTERFACE_ID_INVALID = 0xffffffff; - /* - * bytes4(keccak256('supportsInterface(bytes4)')) == 0x01ffc9a7 - */ - bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7; - /** * @dev Returns true if `account` supports the {IERC165} interface, */ function supportsERC165(address account) internal view returns (bool) { // Any contract that implements ERC165 must explicitly indicate support of // InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid - return _supportsERC165Interface(account, _INTERFACE_ID_ERC165) && + return _supportsERC165Interface(account, type(IERC165).interfaceId) && !_supportsERC165Interface(account, _INTERFACE_ID_INVALID); } @@ -101,29 +98,9 @@ library ERC165Checker { * Interface identification is specified in ERC-165. */ function _supportsERC165Interface(address account, bytes4 interfaceId) private view returns (bool) { - // success determines whether the staticcall succeeded and result determines - // whether the contract at account indicates support of _interfaceId - (bool success, bool result) = _callERC165SupportsInterface(account, interfaceId); - - return (success && result); - } - - /** - * @notice Calls the function with selector 0x01ffc9a7 (ERC165) and suppresses throw - * @param account The address of the contract to query for support of an interface - * @param interfaceId The interface identifier, as specified in ERC-165 - * @return success true if the STATICCALL succeeded, false otherwise - * @return result true if the STATICCALL succeeded and the contract at account - * indicates support of the interface with identifier interfaceId, false otherwise - */ - function _callERC165SupportsInterface(address account, bytes4 interfaceId) - private - view - returns (bool, bool) - { - bytes memory encodedParams = abi.encodeWithSelector(_INTERFACE_ID_ERC165, interfaceId); + bytes memory encodedParams = abi.encodeWithSelector(IERC165(account).supportsInterface.selector, interfaceId); (bool success, bytes memory result) = account.staticcall{ gas: 30000 }(encodedParams); - if (result.length < 32) return (false, false); - return (success, abi.decode(result, (bool))); + if (result.length < 32) return false; + return success && abi.decode(result, (bool)); } } diff --git a/contracts/introspection/ERC1820Implementer.sol b/contracts/introspection/ERC1820Implementer.sol index b14c30ca3..89c87b119 100644 --- a/contracts/introspection/ERC1820Implementer.sol +++ b/contracts/introspection/ERC1820Implementer.sol @@ -13,7 +13,7 @@ import "./IERC1820Implementer.sol"; * registration to be complete. */ contract ERC1820Implementer is IERC1820Implementer { - bytes32 constant private _ERC1820_ACCEPT_MAGIC = keccak256(abi.encodePacked("ERC1820_ACCEPT_MAGIC")); + bytes32 private constant _ERC1820_ACCEPT_MAGIC = keccak256("ERC1820_ACCEPT_MAGIC"); mapping(bytes32 => mapping(address => bool)) private _supportedInterfaces; diff --git a/contracts/mocks/ERC165/ERC165MissingData.sol b/contracts/mocks/ERC165/ERC165MissingData.sol new file mode 100644 index 000000000..59cd51ae6 --- /dev/null +++ b/contracts/mocks/ERC165/ERC165MissingData.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +contract ERC165MissingData { + function supportsInterface(bytes4 interfaceId) public view {} // missing return +} diff --git a/contracts/mocks/ERC721ReceiverMock.sol b/contracts/mocks/ERC721ReceiverMock.sol index eafacd804..0ba5035e8 100644 --- a/contracts/mocks/ERC721ReceiverMock.sol +++ b/contracts/mocks/ERC721ReceiverMock.sol @@ -5,20 +5,34 @@ pragma solidity ^0.8.0; import "../token/ERC721/IERC721Receiver.sol"; contract ERC721ReceiverMock is IERC721Receiver { - bytes4 private _retval; - bool private _reverts; + enum Error { + None, + RevertWithMessage, + RevertWithoutMessage, + Panic + } + + bytes4 private immutable _retval; + Error private immutable _error; event Received(address operator, address from, uint256 tokenId, bytes data, uint256 gas); - constructor (bytes4 retval, bool reverts) { + constructor (bytes4 retval, Error error) { _retval = retval; - _reverts = reverts; + _error = error; } function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data) public override returns (bytes4) { - require(!_reverts, "ERC721ReceiverMock: reverting"); + if (_error == Error.RevertWithMessage) { + revert("ERC721ReceiverMock: reverting"); + } else if (_error == Error.RevertWithoutMessage) { + revert(); + } else if (_error == Error.Panic) { + uint256 a = uint256(0) / uint256(0); + a; + } emit Received(operator, from, tokenId, data, gasleft()); return _retval; } diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index e4c868685..0d2cf2690 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -29,24 +29,6 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI { // Used as the URI for all token types by relying on ID substitution, e.g. https://token-cdn-domain/{id}.json string private _uri; - /* - * bytes4(keccak256('balanceOf(address,uint256)')) == 0x00fdd58e - * bytes4(keccak256('balanceOfBatch(address[],uint256[])')) == 0x4e1273f4 - * bytes4(keccak256('setApprovalForAll(address,bool)')) == 0xa22cb465 - * bytes4(keccak256('isApprovedForAll(address,address)')) == 0xe985e9c5 - * bytes4(keccak256('safeTransferFrom(address,address,uint256,uint256,bytes)')) == 0xf242432a - * bytes4(keccak256('safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)')) == 0x2eb2c2d6 - * - * => 0x00fdd58e ^ 0x4e1273f4 ^ 0xa22cb465 ^ - * 0xe985e9c5 ^ 0xf242432a ^ 0x2eb2c2d6 == 0xd9b67a26 - */ - bytes4 private constant _INTERFACE_ID_ERC1155 = 0xd9b67a26; - - /* - * bytes4(keccak256('uri(uint256)')) == 0x0e89341c - */ - bytes4 private constant _INTERFACE_ID_ERC1155_METADATA_URI = 0x0e89341c; - /** * @dev See {_setURI}. */ @@ -54,10 +36,10 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI { _setURI(uri_); // register the supported interfaces to conform to ERC1155 via ERC165 - _registerInterface(_INTERFACE_ID_ERC1155); + _registerInterface(type(IERC1155).interfaceId); // register the supported interfaces to conform to ERC1155MetadataURI via ERC165 - _registerInterface(_INTERFACE_ID_ERC1155_METADATA_URI); + _registerInterface(type(IERC1155MetadataURI).interfaceId); } /** diff --git a/contracts/token/ERC1155/ERC1155Receiver.sol b/contracts/token/ERC1155/ERC1155Receiver.sol index f3e870270..03a6eb58e 100644 --- a/contracts/token/ERC1155/ERC1155Receiver.sol +++ b/contracts/token/ERC1155/ERC1155Receiver.sol @@ -10,9 +10,6 @@ import "../../introspection/ERC165.sol"; */ abstract contract ERC1155Receiver is ERC165, IERC1155Receiver { constructor() { - _registerInterface( - ERC1155Receiver(address(0)).onERC1155Received.selector ^ - ERC1155Receiver(address(0)).onERC1155BatchReceived.selector - ); + _registerInterface(type(IERC1155Receiver).interfaceId); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index cef88635b..0ca91ee4c 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -23,10 +23,6 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable using EnumerableMap for EnumerableMap.UintToAddressMap; using Strings for uint256; - // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))` - // which can be also obtained as `IERC721Receiver(0).onERC721Received.selector` - bytes4 private constant _ERC721_RECEIVED = 0x150b7a02; - // Mapping from holder address to their (enumerable) set of owned tokens mapping (address => EnumerableSet.UintSet) private _holderTokens; @@ -51,40 +47,6 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable // Base URI string private _baseURI; - /* - * bytes4(keccak256('balanceOf(address)')) == 0x70a08231 - * bytes4(keccak256('ownerOf(uint256)')) == 0x6352211e - * bytes4(keccak256('approve(address,uint256)')) == 0x095ea7b3 - * bytes4(keccak256('getApproved(uint256)')) == 0x081812fc - * bytes4(keccak256('setApprovalForAll(address,bool)')) == 0xa22cb465 - * bytes4(keccak256('isApprovedForAll(address,address)')) == 0xe985e9c5 - * bytes4(keccak256('transferFrom(address,address,uint256)')) == 0x23b872dd - * bytes4(keccak256('safeTransferFrom(address,address,uint256)')) == 0x42842e0e - * bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)')) == 0xb88d4fde - * - * => 0x70a08231 ^ 0x6352211e ^ 0x095ea7b3 ^ 0x081812fc ^ - * 0xa22cb465 ^ 0xe985e9c5 ^ 0x23b872dd ^ 0x42842e0e ^ 0xb88d4fde == 0x80ac58cd - */ - bytes4 private constant _INTERFACE_ID_ERC721 = 0x80ac58cd; - - /* - * bytes4(keccak256('name()')) == 0x06fdde03 - * bytes4(keccak256('symbol()')) == 0x95d89b41 - * bytes4(keccak256('tokenURI(uint256)')) == 0xc87b56dd - * - * => 0x06fdde03 ^ 0x95d89b41 ^ 0xc87b56dd == 0x5b5e139f - */ - bytes4 private constant _INTERFACE_ID_ERC721_METADATA = 0x5b5e139f; - - /* - * bytes4(keccak256('totalSupply()')) == 0x18160ddd - * bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) == 0x2f745c59 - * bytes4(keccak256('tokenByIndex(uint256)')) == 0x4f6ccce7 - * - * => 0x18160ddd ^ 0x2f745c59 ^ 0x4f6ccce7 == 0x780e9d63 - */ - bytes4 private constant _INTERFACE_ID_ERC721_ENUMERABLE = 0x780e9d63; - /** * @dev Initializes the contract by setting a `name` and a `symbol` to the token collection. */ @@ -93,9 +55,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable _symbol = symbol_; // register the supported interfaces to conform to ERC721 via ERC165 - _registerInterface(_INTERFACE_ID_ERC721); - _registerInterface(_INTERFACE_ID_ERC721_METADATA); - _registerInterface(_INTERFACE_ID_ERC721_ENUMERABLE); + _registerInterface(type(IERC721).interfaceId); + _registerInterface(type(IERC721Metadata).interfaceId); + _registerInterface(type(IERC721Enumerable).interfaceId); } /** @@ -433,18 +395,22 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data) private returns (bool) { - if (!to.isContract()) { + if (to.isContract()) { + try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) { + return retval == IERC721Receiver(to).onERC721Received.selector; + } catch (bytes memory reason) { + if (reason.length == 0) { + revert("ERC721: transfer to non ERC721Receiver implementer"); + } else { + // solhint-disable-next-line no-inline-assembly + assembly { + revert(add(32, reason), mload(reason)) + } + } + } + } else { return true; } - bytes memory returndata = to.functionCall(abi.encodeWithSelector( - IERC721Receiver(to).onERC721Received.selector, - _msgSender(), - from, - tokenId, - _data - ), "ERC721: transfer to non ERC721Receiver implementer"); - bytes4 retval = abi.decode(returndata, (bytes4)); - return (retval == _ERC721_RECEIVED); } function _approve(address to, uint256 tokenId) private { diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 7dd43ba6d..e5fa654bf 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -37,16 +37,8 @@ contract ERC777 is Context, IERC777, IERC20 { string private _name; string private _symbol; - // We inline the result of the following hashes because Solidity doesn't resolve them at compile time. - // See https://github.com/ethereum/solidity/issues/4024. - - // keccak256("ERC777TokensSender") - bytes32 constant private _TOKENS_SENDER_INTERFACE_HASH = - 0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895; - - // keccak256("ERC777TokensRecipient") - bytes32 constant private _TOKENS_RECIPIENT_INTERFACE_HASH = - 0xb281fc8c12954d22544db45de3159a39272895b169a852b314f9cc762e44c53b; + bytes32 private constant _TOKENS_SENDER_INTERFACE_HASH = keccak256("ERC777TokensSender"); + bytes32 private constant _TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient"); // This isn't ever read from - it's only used to respond to the defaultOperators query. address[] private _defaultOperatorsArray; diff --git a/test/introspection/ERC165Checker.test.js b/test/introspection/ERC165Checker.test.js index ae2b97e6c..c3a6cdc66 100644 --- a/test/introspection/ERC165Checker.test.js +++ b/test/introspection/ERC165Checker.test.js @@ -3,6 +3,7 @@ require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const ERC165CheckerMock = artifacts.require('ERC165CheckerMock'); +const ERC165MissingData = artifacts.require('ERC165MissingData'); const ERC165NotSupported = artifacts.require('ERC165NotSupported'); const ERC165InterfacesSupported = artifacts.require('ERC165InterfacesSupported'); @@ -18,6 +19,33 @@ contract('ERC165Checker', function (accounts) { this.mock = await ERC165CheckerMock.new(); }); + context('ERC165 missing return data', function () { + beforeEach(async function () { + this.target = await ERC165MissingData.new(); + }); + + it('does not support ERC165', async function () { + const supported = await this.mock.supportsERC165(this.target.address); + expect(supported).to.equal(false); + }); + + it('does not support mock interface via supportsInterface', async function () { + const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID); + expect(supported).to.equal(false); + }); + + it('does not support mock interface via supportsAllInterfaces', async function () { + const supported = await this.mock.supportsAllInterfaces(this.target.address, [DUMMY_ID]); + expect(supported).to.equal(false); + }); + + it('does not support mock interface via getSupportedInterfaces', async function () { + const supported = await this.mock.getSupportedInterfaces(this.target.address, [DUMMY_ID]); + expect(supported.length).to.equal(1); + expect(supported[0]).to.equal(false); + }); + }); + context('ERC165 not supported', function () { beforeEach(async function () { this.target = await ERC165NotSupported.new(); diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js index d67dea215..23f408805 100644 --- a/test/token/ERC721/ERC721.test.js +++ b/test/token/ERC721/ERC721.test.js @@ -8,6 +8,9 @@ const { shouldSupportInterfaces } = require('../../introspection/SupportsInterfa const ERC721Mock = artifacts.require('ERC721Mock'); const ERC721ReceiverMock = artifacts.require('ERC721ReceiverMock'); +const Error = [ 'None', 'RevertWithMessage', 'RevertWithoutMessage', 'Panic' ] + .reduce((acc, entry, idx) => Object.assign({ [entry]: idx }, acc), {}); + contract('ERC721', function (accounts) { const [owner, newOwner, approved, anotherApproved, operator, other] = accounts; @@ -324,7 +327,7 @@ contract('ERC721', function (accounts) { describe('to a valid receiver contract', function () { beforeEach(async function () { - this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false); + this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None); this.toWhom = this.receiver.address; }); @@ -379,7 +382,7 @@ contract('ERC721', function (accounts) { describe('to a receiver contract returning unexpected value', function () { it('reverts', async function () { - const invalidReceiver = await ERC721ReceiverMock.new('0x42', false); + const invalidReceiver = await ERC721ReceiverMock.new('0x42', Error.None); await expectRevert( this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }), 'ERC721: transfer to non ERC721Receiver implementer', @@ -387,9 +390,9 @@ contract('ERC721', function (accounts) { }); }); - describe('to a receiver contract that throws', function () { + describe('to a receiver contract that reverts with message', function () { it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithMessage); await expectRevert( this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), 'ERC721ReceiverMock: reverting', @@ -397,6 +400,25 @@ contract('ERC721', function (accounts) { }); }); + describe('to a receiver contract that reverts without message', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithoutMessage); + await expectRevert( + this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), + 'ERC721: transfer to non ERC721Receiver implementer', + ); + }); + }); + + describe('to a receiver contract that panics', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.Panic); + await expectRevert.unspecified( + this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), + ); + }); + }); + describe('to a contract that does not implement the required function', function () { it('reverts', async function () { const nonReceiver = this.token; @@ -416,7 +438,7 @@ contract('ERC721', function (accounts) { describe('via safeMint', function () { // regular minting is tested in ERC721Mintable.test.js and others it('calls onERC721Received — with data', async function () { - this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false); + this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None); const receipt = await this.token.safeMint(this.receiver.address, tokenId, data); await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { @@ -427,7 +449,7 @@ contract('ERC721', function (accounts) { }); it('calls onERC721Received — without data', async function () { - this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, false); + this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.None); const receipt = await this.token.safeMint(this.receiver.address, tokenId); await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { @@ -438,7 +460,7 @@ contract('ERC721', function (accounts) { context('to a receiver contract returning unexpected value', function () { it('reverts', async function () { - const invalidReceiver = await ERC721ReceiverMock.new('0x42', false); + const invalidReceiver = await ERC721ReceiverMock.new('0x42', Error.None); await expectRevert( this.token.safeMint(invalidReceiver.address, tokenId), 'ERC721: transfer to non ERC721Receiver implementer', @@ -446,9 +468,9 @@ contract('ERC721', function (accounts) { }); }); - context('to a receiver contract that throws', function () { + context('to a receiver contract that reverts with message', function () { it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, true); + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithMessage); await expectRevert( this.token.safeMint(revertingReceiver.address, tokenId), 'ERC721ReceiverMock: reverting', @@ -456,6 +478,25 @@ contract('ERC721', function (accounts) { }); }); + context('to a receiver contract that reverts without message', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.RevertWithoutMessage); + await expectRevert( + this.token.safeMint(revertingReceiver.address, tokenId), + 'ERC721: transfer to non ERC721Receiver implementer', + ); + }); + }); + + context('to a receiver contract that panics', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, Error.Panic); + await expectRevert.unspecified( + this.token.safeMint(revertingReceiver.address, tokenId), + ); + }); + }); + context('to a contract that does not implement the required function', function () { it('reverts', async function () { const nonReceiver = this.token;