diff --git a/.changeset/mighty-donuts-smile.md b/.changeset/mighty-donuts-smile.md new file mode 100644 index 00000000000..5885a73705d --- /dev/null +++ b/.changeset/mighty-donuts-smile.md @@ -0,0 +1,6 @@ +--- +'openzeppelin-solidity': patch +--- + +`Governor`: Add validation in ERC1155 and ERC721 receiver hooks to ensure Governor is the executor. + diff --git a/.changeset/thin-camels-matter.md b/.changeset/thin-camels-matter.md new file mode 100644 index 00000000000..c832b116371 --- /dev/null +++ b/.changeset/thin-camels-matter.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC1155`: Bubble errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index b42fe1034dc..13a5db52492 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -70,7 +70,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive * governance protocol (since v4.6). */ modifier onlyGovernance() { - if (_msgSender() != _executor()) { + if (_executor() != _msgSender()) { revert GovernorOnlyExecutor(_msgSender()); } if (_executor() != address(this)) { @@ -631,20 +631,29 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive /** * @dev See {IERC721Receiver-onERC721Received}. + * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock). */ function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) { + if (_executor() != address(this)) { + revert GovernorDisabledDeposit(); + } return this.onERC721Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155Received}. + * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock). */ function onERC1155Received(address, address, uint256, uint256, bytes memory) public virtual returns (bytes4) { + if (_executor() != address(this)) { + revert GovernorDisabledDeposit(); + } return this.onERC1155Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock). */ function onERC1155BatchReceived( address, @@ -653,6 +662,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive uint256[] memory, bytes memory ) public virtual returns (bytes4) { + if (_executor() != address(this)) { + revert GovernorDisabledDeposit(); + } return this.onERC1155BatchReceived.selector; } diff --git a/contracts/mocks/token/ERC1155ReceiverMock.sol b/contracts/mocks/token/ERC1155ReceiverMock.sol index 2b591c058b9..c4cdbf20e4b 100644 --- a/contracts/mocks/token/ERC1155ReceiverMock.sol +++ b/contracts/mocks/token/ERC1155ReceiverMock.sol @@ -6,15 +6,24 @@ import "../../token/ERC1155/IERC1155Receiver.sol"; import "../../utils/introspection/ERC165.sol"; contract ERC1155ReceiverMock is ERC165, IERC1155Receiver { + enum RevertType { + None, + Empty, + String, + Custom + } + bytes4 private _recRetval; - bool private _recReverts; + RevertType private _recReverts; bytes4 private _batRetval; - bool private _batReverts; + RevertType private _batReverts; event Received(address operator, address from, uint256 id, uint256 value, bytes data, uint256 gas); event BatchReceived(address operator, address from, uint256[] ids, uint256[] values, bytes data, uint256 gas); - constructor(bytes4 recRetval, bool recReverts, bytes4 batRetval, bool batReverts) { + error ERC1155ReceiverMockError(); + + constructor(bytes4 recRetval, RevertType recReverts, bytes4 batRetval, RevertType batReverts) { _recRetval = recRetval; _recReverts = recReverts; _batRetval = batRetval; @@ -28,7 +37,14 @@ contract ERC1155ReceiverMock is ERC165, IERC1155Receiver { uint256 value, bytes calldata data ) external returns (bytes4) { - require(!_recReverts, "ERC1155ReceiverMock: reverting on receive"); + if (_recReverts == RevertType.Empty) { + revert(); + } else if (_recReverts == RevertType.String) { + revert("ERC1155ReceiverMock: reverting on receive"); + } else if (_recReverts == RevertType.Custom) { + revert ERC1155ReceiverMockError(); + } + emit Received(operator, from, id, value, data, gasleft()); return _recRetval; } @@ -40,7 +56,14 @@ contract ERC1155ReceiverMock is ERC165, IERC1155Receiver { uint256[] calldata values, bytes calldata data ) external returns (bytes4) { - require(!_batReverts, "ERC1155ReceiverMock: reverting on batch receive"); + if (_batReverts == RevertType.Empty) { + revert(); + } else if (_batReverts == RevertType.String) { + revert("ERC1155ReceiverMock: reverting on batch receive"); + } else if (_batReverts == RevertType.Custom) { + revert ERC1155ReceiverMockError(); + } + emit BatchReceived(operator, from, ids, values, data, gasleft()); return _batRetval; } diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index 05bcc3fa1bd..2e9ffacc816 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -360,11 +360,16 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IERC1155Erro // Tokens rejected revert ERC1155InvalidReceiver(to); } - } catch Error(string memory reason) { - revert(reason); - } catch { - // non-ERC1155Receiver implementer - revert ERC1155InvalidReceiver(to); + } catch (bytes memory reason) { + if (reason.length == 0) { + // non-ERC1155Receiver implementer + revert ERC1155InvalidReceiver(to); + } else { + /// @solidity memory-safe-assembly + assembly { + revert(add(32, reason), mload(reason)) + } + } } } } @@ -385,11 +390,16 @@ contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IERC1155Erro // Tokens rejected revert ERC1155InvalidReceiver(to); } - } catch Error(string memory reason) { - revert(reason); - } catch { - // non-ERC1155Receiver implementer - revert ERC1155InvalidReceiver(to); + } catch (bytes memory reason) { + if (reason.length == 0) { + // non-ERC1155Receiver implementer + revert ERC1155InvalidReceiver(to); + } else { + /// @solidity memory-safe-assembly + assembly { + revert(add(32, reason), mload(reason)) + } + } } } } diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 8c6680aa83c..c406baf8dde 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -11,6 +11,8 @@ const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsI const Timelock = artifacts.require('CompTimelock'); const Governor = artifacts.require('$GovernorTimelockCompoundMock'); const CallReceiver = artifacts.require('CallReceiverMock'); +const ERC721 = artifacts.require('$ERC721'); +const ERC1155 = artifacts.require('$ERC1155'); function makeContractAddress(creator, nonce) { return web3.utils.toChecksumAddress( @@ -212,6 +214,70 @@ contract('GovernorTimelockCompound', function (accounts) { ]); }); }); + + describe('on safe receive', function () { + describe('ERC721', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + const tokenId = web3.utils.toBN(1); + + beforeEach(async function () { + this.token = await ERC721.new(name, symbol); + await this.token.$_mint(owner, tokenId); + }); + + it("can't receive an ERC721 safeTransfer", async function () { + await expectRevertCustomError( + this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + 'GovernorDisabledDeposit', + [], + ); + }); + }); + + describe('ERC1155', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const tokenIds = { + 1: web3.utils.toBN(1000), + 2: web3.utils.toBN(2000), + 3: web3.utils.toBN(3000), + }; + + beforeEach(async function () { + this.token = await ERC1155.new(uri); + await this.token.$_mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x'); + }); + + it("can't receive ERC1155 safeTransfer", async function () { + await expectRevertCustomError( + this.token.safeTransferFrom( + owner, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: owner }, + ), + 'GovernorDisabledDeposit', + [], + ); + }); + + it("can't receive ERC1155 safeBatchTransfer", async function () { + await expectRevertCustomError( + this.token.safeBatchTransferFrom( + owner, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: owner }, + ), + 'GovernorDisabledDeposit', + [], + ); + }); + }); + }); }); describe('cancel', function () { diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index ef32148faf8..5802893f00a 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -10,6 +10,8 @@ const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsI const Timelock = artifacts.require('TimelockController'); const Governor = artifacts.require('$GovernorTimelockControlMock'); const CallReceiver = artifacts.require('CallReceiverMock'); +const ERC721 = artifacts.require('$ERC721'); +const ERC1155 = artifacts.require('$ERC1155'); const TOKENS = [ { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, @@ -466,6 +468,70 @@ contract('GovernorTimelockControl', function (accounts) { // then coverage reports. }); }); + + describe('on safe receive', function () { + describe('ERC721', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + const tokenId = web3.utils.toBN(1); + + beforeEach(async function () { + this.token = await ERC721.new(name, symbol); + await this.token.$_mint(owner, tokenId); + }); + + it("can't receive an ERC721 safeTransfer", async function () { + await expectRevertCustomError( + this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + 'GovernorDisabledDeposit', + [], + ); + }); + }); + + describe('ERC1155', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const tokenIds = { + 1: web3.utils.toBN(1000), + 2: web3.utils.toBN(2000), + 3: web3.utils.toBN(3000), + }; + + beforeEach(async function () { + this.token = await ERC1155.new(uri); + await this.token.$_mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x'); + }); + + it("can't receive ERC1155 safeTransfer", async function () { + await expectRevertCustomError( + this.token.safeTransferFrom( + owner, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: owner }, + ), + 'GovernorDisabledDeposit', + [], + ); + }); + + it("can't receive ERC1155 safeBatchTransfer", async function () { + await expectRevertCustomError( + this.token.safeBatchTransferFrom( + owner, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: owner }, + ), + 'GovernorDisabledDeposit', + [], + ); + }); + }); + }); }); }); } diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index 5e87f8d5272..fe537a53b0a 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -5,8 +5,10 @@ const { expect } = require('chai'); const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { Enum } = require('../../helpers/enums'); const ERC1155ReceiverMock = artifacts.require('ERC1155ReceiverMock'); +const RevertType = Enum('None', 'Empty', 'String', 'Custom'); function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, multiTokenHolder, recipient, proxy]) { const firstTokenId = new BN(1); @@ -296,9 +298,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m beforeEach(async function () { this.receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - false, + RevertType.None, RECEIVER_BATCH_MAGIC_VALUE, - false, + RevertType.None, ); }); @@ -370,7 +372,12 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m context('to a receiver contract returning unexpected value', function () { beforeEach(async function () { - this.receiver = await ERC1155ReceiverMock.new('0x00c0ffee', false, RECEIVER_BATCH_MAGIC_VALUE, false); + this.receiver = await ERC1155ReceiverMock.new( + '0x00c0ffee', + RevertType.None, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.None, + ); }); it('reverts', async function () { @@ -385,23 +392,55 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m }); context('to a receiver contract that reverts', function () { - beforeEach(async function () { - this.receiver = await ERC1155ReceiverMock.new( + it('with empty reason', async function () { + const receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - true, + RevertType.Empty, RECEIVER_BATCH_MAGIC_VALUE, - false, + RevertType.None, + ); + + await expectRevertCustomError( + this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', { + from: multiTokenHolder, + }), + 'ERC1155InvalidReceiver', + [receiver.address], ); }); - it('reverts', async function () { + it('with reason string', async function () { + const receiver = await ERC1155ReceiverMock.new( + RECEIVER_SINGLE_MAGIC_VALUE, + RevertType.String, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.None, + ); + await expectRevert( - this.token.safeTransferFrom(multiTokenHolder, this.receiver.address, firstTokenId, firstAmount, '0x', { + this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', { from: multiTokenHolder, }), 'ERC1155ReceiverMock: reverting on receive', ); }); + + it('with custom error', async function () { + const receiver = await ERC1155ReceiverMock.new( + RECEIVER_SINGLE_MAGIC_VALUE, + RevertType.Custom, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.None, + ); + + await expectRevertCustomError( + this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', { + from: multiTokenHolder, + }), + 'ERC1155ReceiverMockError', + [], + ); + }); }); context('to a contract that does not implement the required function', function () { @@ -582,9 +621,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m beforeEach(async function () { this.receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - false, + RevertType.None, RECEIVER_BATCH_MAGIC_VALUE, - false, + RevertType.None, ); }); @@ -658,9 +697,9 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m beforeEach(async function () { this.receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - false, + RevertType.None, RECEIVER_SINGLE_MAGIC_VALUE, - false, + RevertType.None, ); }); @@ -681,20 +720,40 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m }); context('to a receiver contract that reverts', function () { - beforeEach(async function () { - this.receiver = await ERC1155ReceiverMock.new( + it('with empty reason', async function () { + const receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - false, + RevertType.None, RECEIVER_BATCH_MAGIC_VALUE, - true, + RevertType.Empty, + ); + + await expectRevertCustomError( + this.token.safeBatchTransferFrom( + multiTokenHolder, + receiver.address, + [firstTokenId, secondTokenId], + [firstAmount, secondAmount], + '0x', + { from: multiTokenHolder }, + ), + 'ERC1155InvalidReceiver', + [receiver.address], ); }); - it('reverts', async function () { + it('with reason string', async function () { + const receiver = await ERC1155ReceiverMock.new( + RECEIVER_SINGLE_MAGIC_VALUE, + RevertType.None, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.String, + ); + await expectRevert( this.token.safeBatchTransferFrom( multiTokenHolder, - this.receiver.address, + receiver.address, [firstTokenId, secondTokenId], [firstAmount, secondAmount], '0x', @@ -703,15 +762,37 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m 'ERC1155ReceiverMock: reverting on batch receive', ); }); + + it('with custom error', async function () { + const receiver = await ERC1155ReceiverMock.new( + RECEIVER_SINGLE_MAGIC_VALUE, + RevertType.None, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.Custom, + ); + + await expectRevertCustomError( + this.token.safeBatchTransferFrom( + multiTokenHolder, + receiver.address, + [firstTokenId, secondTokenId], + [firstAmount, secondAmount], + '0x', + { from: multiTokenHolder }, + ), + 'ERC1155ReceiverMockError', + [], + ); + }); }); context('to a receiver contract that reverts only on single transfers', function () { beforeEach(async function () { this.receiver = await ERC1155ReceiverMock.new( RECEIVER_SINGLE_MAGIC_VALUE, - true, + RevertType.String, RECEIVER_BATCH_MAGIC_VALUE, - false, + RevertType.None, ); this.toWhom = this.receiver.address;