From 47402f9153cd16939c29a299ea78cc07c09caab3 Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 5 Jun 2023 10:45:57 +0200 Subject: [PATCH 01/13] add validation in governor receive hooks --- contracts/governance/Governor.sol | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 4ff8c34924d..e8898e0c383 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -603,14 +603,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive } /** - * @dev See {IERC721Receiver-onERC721Received}. + * @dev See {IERC721Receiver-onERC721Received}(disabled if executor is a third party contract). */ function onERC721Received(address, address, uint256, bytes memory) public virtual override returns (bytes4) { + require(_executor() == address(this), "Governor: must send to executor"); return this.onERC721Received.selector; } /** - * @dev See {IERC1155Receiver-onERC1155Received}. + * @dev See {IERC1155Receiver-onERC1155Received}(disabled if executor is a third party contract). */ function onERC1155Received( address, @@ -619,11 +620,12 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive uint256, bytes memory ) public virtual override returns (bytes4) { + require(_executor() == address(this), "Governor: must send to executor"); return this.onERC1155Received.selector; } /** - * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + * @dev See {IERC1155Receiver-onERC1155BatchReceived}(disabled if executor is a third party contract). */ function onERC1155BatchReceived( address, @@ -632,6 +634,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive uint256[] memory, bytes memory ) public virtual override returns (bytes4) { + require(_executor() == address(this), "Governor: must send to executor"); return this.onERC1155BatchReceived.selector; } } From fa170402e49ddc2122ada1fe8ad915a47e0c764d Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 5 Jun 2023 10:53:06 +0200 Subject: [PATCH 02/13] add test cases for governor receive hooks validations --- .../GovernorTimelockCompound.test.js | 60 +++++++++++++++++++ .../GovernorTimelockControl.test.js | 60 +++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 2cbce26000b..514ecffac55 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -10,6 +10,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( @@ -202,6 +204,64 @@ contract('GovernorTimelockCompound', function (accounts) { await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); }); }); + + 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 expectRevert.unspecified( + this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + ); + }); + }); + + 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 expectRevert.unspecified( + this.token.safeTransferFrom( + owner, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: owner }, + ), + ); + }); + + it("can't receive ERC1155 safeBatchTransfer", async function () { + await expectRevert.unspecified( + this.token.safeBatchTransferFrom( + owner, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: owner }, + ), + ); + }); + }); + }); }); describe('cancel', function () { diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index af57ba90b58..cf8399147c9 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -9,6 +9,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' }, @@ -439,6 +441,64 @@ 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 expectRevert.unspecified( + this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + ); + }); + }); + + 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 expectRevert.unspecified( + this.token.safeTransferFrom( + owner, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: owner }, + ), + ); + }); + + it("can't receive ERC1155 safeBatchTransfer", async function () { + await expectRevert.unspecified( + this.token.safeBatchTransferFrom( + owner, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: owner }, + ), + ); + }); + }); + }); }); }); } From 40139a807f550cd97e710c3378e3711dcd6217fc Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 5 Jun 2023 12:18:46 +0200 Subject: [PATCH 03/13] add changeset --- .changeset/mighty-donuts-smile.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mighty-donuts-smile.md diff --git a/.changeset/mighty-donuts-smile.md b/.changeset/mighty-donuts-smile.md new file mode 100644 index 00000000000..3badb1febbe --- /dev/null +++ b/.changeset/mighty-donuts-smile.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +Add validation in Governor ERC1155 and ERC721 receiver hooks to ensure Governor is the executor From 38c47595c10a2e39ecc416e9f1985f5693deb0ab Mon Sep 17 00:00:00 2001 From: Claudia Date: Mon, 5 Jun 2023 16:15:26 +0200 Subject: [PATCH 04/13] add revert message in governor timelock test --- .../extensions/GovernorTimelockCompound.test.js | 9 ++++++--- .../extensions/GovernorTimelockControl.test.js | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 514ecffac55..422cdae8de0 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -217,8 +217,9 @@ contract('GovernorTimelockCompound', function (accounts) { }); it("can't receive an ERC721 safeTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + 'Governor: must send to executor', ); }); }); @@ -237,7 +238,7 @@ contract('GovernorTimelockCompound', function (accounts) { }); it("can't receive ERC1155 safeTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeTransferFrom( owner, this.mock.address, @@ -245,11 +246,12 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), + 'Governor: must send to executor', ); }); it("can't receive ERC1155 safeBatchTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeBatchTransferFrom( owner, this.mock.address, @@ -258,6 +260,7 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), + 'Governor: must send to executor', ); }); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index cf8399147c9..861d2e6dde9 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -454,8 +454,9 @@ contract('GovernorTimelockControl', function (accounts) { }); it("can't receive an ERC721 safeTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), + 'Governor: must send to executor', ); }); }); @@ -474,7 +475,7 @@ contract('GovernorTimelockControl', function (accounts) { }); it("can't receive ERC1155 safeTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeTransferFrom( owner, this.mock.address, @@ -482,11 +483,12 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), + 'Governor: must send to executor', ); }); it("can't receive ERC1155 safeBatchTransfer", async function () { - await expectRevert.unspecified( + await expectRevert( this.token.safeBatchTransferFrom( owner, this.mock.address, @@ -495,6 +497,7 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), + 'Governor: must send to executor', ); }); }); From 6a8d20fab2e725e83d3d47f5431d2aee02b0997d Mon Sep 17 00:00:00 2001 From: Claudia Date: Tue, 6 Jun 2023 10:51:43 +0200 Subject: [PATCH 05/13] change hooks comments for more descriptive ones --- contracts/governance/Governor.sol | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index e8898e0c383..8feeb430c07 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -603,7 +603,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive } /** - * @dev See {IERC721Receiver-onERC721Received}(disabled if executor is a third party contract). + * @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 override returns (bytes4) { require(_executor() == address(this), "Governor: must send to executor"); @@ -611,7 +612,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive } /** - * @dev See {IERC1155Receiver-onERC1155Received}(disabled if executor is a third party contract). + * @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, @@ -625,7 +627,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive } /** - * @dev See {IERC1155Receiver-onERC1155BatchReceived}(disabled if executor is a third party contract). + * @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, From d07c900809d40379fada41f16206f1175a8100a7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jun 2023 12:09:58 +0200 Subject: [PATCH 06/13] Fix tests --- .../extensions/GovernorTimelockCompound.test.js | 15 +++++++++------ .../extensions/GovernorTimelockControl.test.js | 15 +++++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index e038e48aa39..014505a99c7 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -227,9 +227,10 @@ contract('GovernorTimelockCompound', function (accounts) { }); it("can't receive an ERC721 safeTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), - 'Governor: must send to executor', + 'GovernorDisabledDeposit', + [], ); }); }); @@ -248,7 +249,7 @@ contract('GovernorTimelockCompound', function (accounts) { }); it("can't receive ERC1155 safeTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeTransferFrom( owner, this.mock.address, @@ -256,12 +257,13 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), - 'Governor: must send to executor', + 'ERC1155InvalidReceiver', + [this.mock.address], ); }); it("can't receive ERC1155 safeBatchTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeBatchTransferFrom( owner, this.mock.address, @@ -270,7 +272,8 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), - 'Governor: must send to executor', + 'ERC1155InvalidReceiver', + [this.mock.address], ); }); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 99bdf74a266..f0bc571445c 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -481,9 +481,10 @@ contract('GovernorTimelockControl', function (accounts) { }); it("can't receive an ERC721 safeTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }), - 'Governor: must send to executor', + 'GovernorDisabledDeposit', + [], ); }); }); @@ -502,7 +503,7 @@ contract('GovernorTimelockControl', function (accounts) { }); it("can't receive ERC1155 safeTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeTransferFrom( owner, this.mock.address, @@ -510,12 +511,13 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), - 'Governor: must send to executor', + 'ERC1155InvalidReceiver', + [this.mock.address], ); }); it("can't receive ERC1155 safeBatchTransfer", async function () { - await expectRevert( + await expectRevertCustomError( this.token.safeBatchTransferFrom( owner, this.mock.address, @@ -524,7 +526,8 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), - 'Governor: must send to executor', + 'ERC1155InvalidReceiver', + [this.mock.address], ); }); }); From 8e37c00fda382e278a35071d1f4af1bd2ce07c36 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jun 2023 13:52:55 +0200 Subject: [PATCH 07/13] Bubble custom errors emitted in ERC1155Receiver hooks --- contracts/mocks/token/ERC1155ReceiverMock.sol | 33 ++++- contracts/token/ERC1155/ERC1155.sol | 30 +++-- .../GovernorTimelockCompound.test.js | 8 +- .../GovernorTimelockControl.test.js | 8 +- test/token/ERC1155/ERC1155.behavior.js | 118 ++++++++++++++---- 5 files changed, 153 insertions(+), 44 deletions(-) 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 014505a99c7..c406baf8dde 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -257,8 +257,8 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), - 'ERC1155InvalidReceiver', - [this.mock.address], + 'GovernorDisabledDeposit', + [], ); }); @@ -272,8 +272,8 @@ contract('GovernorTimelockCompound', function (accounts) { '0x', { from: owner }, ), - 'ERC1155InvalidReceiver', - [this.mock.address], + 'GovernorDisabledDeposit', + [], ); }); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index f0bc571445c..5802893f00a 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -511,8 +511,8 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), - 'ERC1155InvalidReceiver', - [this.mock.address], + 'GovernorDisabledDeposit', + [], ); }); @@ -526,8 +526,8 @@ contract('GovernorTimelockControl', function (accounts) { '0x', { from: owner }, ), - 'ERC1155InvalidReceiver', - [this.mock.address], + 'GovernorDisabledDeposit', + [], ); }); }); diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index 5e87f8d5272..f423e52f97d 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,7 @@ 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 +387,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 expectRevert( + 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 +616,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 +692,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 +715,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 +757,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; From 8a244e5cbe00d95d7c462b3b88fd307e4910ab78 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jun 2023 16:29:46 +0200 Subject: [PATCH 08/13] add changeset --- .changeset/thin-camels-matter.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/thin-camels-matter.md diff --git a/.changeset/thin-camels-matter.md b/.changeset/thin-camels-matter.md new file mode 100644 index 00000000000..6d9ef85e461 --- /dev/null +++ b/.changeset/thin-camels-matter.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC1155`: bubble custom errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks. From e8370d56d4db4f0ea0cc37f10d03e9e8cb12a4b0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jun 2023 16:30:25 +0200 Subject: [PATCH 09/13] fix lint --- test/token/ERC1155/ERC1155.behavior.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index f423e52f97d..170eab45c8f 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -372,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', RevertType.None, RECEIVER_BATCH_MAGIC_VALUE, RevertType.None); + this.receiver = await ERC1155ReceiverMock.new( + '0x00c0ffee', + RevertType.None, + RECEIVER_BATCH_MAGIC_VALUE, + RevertType.None, + ); }); it('reverts', async function () { From b930f65aed164090a15e5e6ffa6a2109a3467fbf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jun 2023 18:35:40 +0200 Subject: [PATCH 10/13] Apply suggestions from code review Co-authored-by: Claudia Barcelo --- test/token/ERC1155/ERC1155.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index 170eab45c8f..fe537a53b0a 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -433,7 +433,7 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m RevertType.None, ); - await expectRevert( + await expectRevertCustomError( this.token.safeTransferFrom(multiTokenHolder, receiver.address, firstTokenId, firstAmount, '0x', { from: multiTokenHolder, }), From 34dd8c0721e65a57d326b4fa58457fc8756c7ce8 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 14 Jun 2023 22:46:42 -0300 Subject: [PATCH 11/13] Update mighty-donuts-smile.md --- .changeset/mighty-donuts-smile.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/mighty-donuts-smile.md b/.changeset/mighty-donuts-smile.md index 3badb1febbe..5885a73705d 100644 --- a/.changeset/mighty-donuts-smile.md +++ b/.changeset/mighty-donuts-smile.md @@ -2,4 +2,5 @@ 'openzeppelin-solidity': patch --- -Add validation in Governor ERC1155 and ERC721 receiver hooks to ensure Governor is the executor +`Governor`: Add validation in ERC1155 and ERC721 receiver hooks to ensure Governor is the executor. + From 40068a28c940d146421dca9a7013bd6e1d73c6bd Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 14 Jun 2023 22:46:58 -0300 Subject: [PATCH 12/13] Update thin-camels-matter.md --- .changeset/thin-camels-matter.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/thin-camels-matter.md b/.changeset/thin-camels-matter.md index 6d9ef85e461..c934e769d77 100644 --- a/.changeset/thin-camels-matter.md +++ b/.changeset/thin-camels-matter.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`ERC1155`: bubble custom errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks. +`ERC1155`: Bubble custom errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks. From 2d53fa77280f95f28c01b49d2fcbf7db5b1f3d64 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 15 Jun 2023 21:42:56 -0300 Subject: [PATCH 13/13] Update .changeset/thin-camels-matter.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- .changeset/thin-camels-matter.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/thin-camels-matter.md b/.changeset/thin-camels-matter.md index c934e769d77..c832b116371 100644 --- a/.changeset/thin-camels-matter.md +++ b/.changeset/thin-camels-matter.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`ERC1155`: Bubble custom errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks. +`ERC1155`: Bubble errors triggered in the `onERC1155Received` and `onERC1155BatchReceived` hooks.