diff --git a/.changeset/vast-worlds-pull.md b/.changeset/vast-worlds-pull.md new file mode 100644 index 00000000000..b0b24f35238 --- /dev/null +++ b/.changeset/vast-worlds-pull.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC1155`: Call `IERC1155Receiver.onERC1155BatchReceived` when performing a batch transfers with exactly one id/value in the batch. diff --git a/CHANGELOG.md b/CHANGELOG.md index bcae40142c6..479e13e99a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Breaking changes +- `ERC1155`: Performing batch transfers with exactly one id/value in the batch no-longer calls `IERC1155Receiver.onERC1155Received`. `IERC1155Receiver.onERC1155BatchReceived` is called instead (with arrays of length one). ([#6170](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6170)) - `ERC1967Proxy` and `TransparentUpgradeableProxy`: Mandate initialization during construction. Deployment now reverts with `ERC1967ProxyUninitialized` if an initialize call is not provided. Developers that rely on the previous behavior and want to disable this check can do so by overriding the internal `_unsafeAllowUninitialized` function to return true. ([#5906](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5906)) - `ERC721` and `ERC1155`: Prevent setting an operator for `address(0)`. In the case of `ERC721` this type of operator allowance could lead to obfuscated mint permission. ([#6171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6171)) - `RLP`: The `encode(bytes32)` function now encodes `bytes32` as a fixed size item and not as a scalar in `encode(uint256)`. Users must replace calls to `encode(bytes32)` with `encode(uint256(bytes32))` to preserve the same behavior. ([#6167](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6167)) diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index 88ced2a6b23..1271333e439 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -178,6 +178,9 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER * IMPORTANT: Overriding this function is discouraged because it poses a reentrancy risk from the receiver. So any * update to the contract state after this function would break the check-effect-interaction pattern. Consider * overriding {_update} instead. + * + * NOTE: This version is kept for backward compatibility. We recommend calling the alternative version with a boolean + * flag in order to achieve better control over which hook to call. */ function _updateWithAcceptanceCheck( address from, @@ -185,16 +188,36 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER uint256[] memory ids, uint256[] memory values, bytes memory data + ) internal virtual { + _updateWithAcceptanceCheck(from, to, ids, values, data, ids.length != 1); + } + + /** + * @dev Version of {_update} that performs the token acceptance check by calling + * {IERC1155Receiver-onERC1155Received} or {IERC1155Receiver-onERC1155BatchReceived} on the receiver address if it + * contains code (eg. is a smart contract at the moment of execution). + * + * IMPORTANT: Overriding this function is discouraged because it poses a reentrancy risk from the receiver. So any + * update to the contract state after this function would break the check-effect-interaction pattern. Consider + * overriding {_update} instead. + */ + function _updateWithAcceptanceCheck( + address from, + address to, + uint256[] memory ids, + uint256[] memory values, + bytes memory data, + bool batch ) internal virtual { _update(from, to, ids, values); if (to != address(0)) { address operator = _msgSender(); - if (ids.length == 1) { + if (batch) { + ERC1155Utils.checkOnERC1155BatchReceived(operator, from, to, ids, values, data); + } else { uint256 id = ids.unsafeMemoryAccess(0); uint256 value = values.unsafeMemoryAccess(0); ERC1155Utils.checkOnERC1155Received(operator, from, to, id, value, data); - } else { - ERC1155Utils.checkOnERC1155BatchReceived(operator, from, to, ids, values, data); } } } @@ -219,7 +242,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER revert ERC1155InvalidSender(address(0)); } (uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value); - _updateWithAcceptanceCheck(from, to, ids, values, data); + _updateWithAcceptanceCheck(from, to, ids, values, data, false); } /** @@ -246,7 +269,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER if (from == address(0)) { revert ERC1155InvalidSender(address(0)); } - _updateWithAcceptanceCheck(from, to, ids, values, data); + _updateWithAcceptanceCheck(from, to, ids, values, data, true); } /** @@ -288,7 +311,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER revert ERC1155InvalidReceiver(address(0)); } (uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value); - _updateWithAcceptanceCheck(address(0), to, ids, values, data); + _updateWithAcceptanceCheck(address(0), to, ids, values, data, false); } /** @@ -307,7 +330,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER if (to == address(0)) { revert ERC1155InvalidReceiver(address(0)); } - _updateWithAcceptanceCheck(address(0), to, ids, values, data); + _updateWithAcceptanceCheck(address(0), to, ids, values, data, true); } /** @@ -325,7 +348,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER revert ERC1155InvalidSender(address(0)); } (uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value); - _updateWithAcceptanceCheck(from, address(0), ids, values, ""); + _updateWithAcceptanceCheck(from, address(0), ids, values, "", false); } /** @@ -343,7 +366,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER if (from == address(0)) { revert ERC1155InvalidSender(address(0)); } - _updateWithAcceptanceCheck(from, address(0), ids, values, ""); + _updateWithAcceptanceCheck(from, address(0), ids, values, "", true); } /** diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index d19b73251a5..22f3ac6c40a 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -147,6 +147,24 @@ function shouldBehaveLikeERC1155() { .withArgs(this.holder, firstTokenValue, firstTokenValue + 1n, firstTokenId); }); + it('reverts when transferring from zero address', async function () { + await expect( + this.token + .connect(this.holder) + .safeTransferFrom(ethers.ZeroAddress, this.holder, firstTokenId, firstTokenValue, '0x'), + ) + .to.be.revertedWithCustomError(this.token, 'ERC1155MissingApprovalForAll') + .withArgs(this.holder, ethers.ZeroAddress); + + await expect( + this.token + .connect(this.holder) + .$_safeTransferFrom(ethers.ZeroAddress, this.holder, firstTokenId, firstTokenValue, '0x'), + ) + .to.be.revertedWithCustomError(this.token, 'ERC1155InvalidSender') + .withArgs(ethers.ZeroAddress); + }); + it('reverts when transferring to zero address', async function () { await expect( this.token @@ -442,6 +460,36 @@ function shouldBehaveLikeERC1155() { .withArgs(ids2.length, tokenValues2.length); }); + it('reverts when transferring from zero address', async function () { + await expect( + this.token + .connect(this.holder) + .safeBatchTransferFrom( + ethers.ZeroAddress, + this.holder, + [firstTokenId, secondTokenId], + [firstTokenValue, secondTokenValue], + '0x', + ), + ) + .to.be.revertedWithCustomError(this.token, 'ERC1155MissingApprovalForAll') + .withArgs(this.holder, ethers.ZeroAddress); + + await expect( + this.token + .connect(this.holder) + .$_safeBatchTransferFrom( + ethers.ZeroAddress, + this.holder, + [firstTokenId, secondTokenId], + [firstTokenValue, secondTokenValue], + '0x', + ), + ) + .to.be.revertedWithCustomError(this.token, 'ERC1155InvalidSender') + .withArgs(ethers.ZeroAddress); + }); + it('reverts when transferring to zero address', async function () { await expect( this.token @@ -484,9 +532,15 @@ function shouldBehaveLikeERC1155() { }); it('emits a TransferBatch log', async function () { - await expect(this.tx) - .to.emit(this.token, 'TransferBatch') - .withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids, this.args.values); + if (this.args.ids.length == 1) { + await expect(this.tx) + .to.emit(this.token, 'TransferSingle') + .withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids[0], this.args.values[0]); + } else { + await expect(this.tx) + .to.emit(this.token, 'TransferBatch') + .withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids, this.args.values); + } }); } @@ -566,7 +620,31 @@ function shouldBehaveLikeERC1155() { ]); }); - describe('without data', function () { + describe('without data (batch of size = 1)', function () { + beforeEach(async function () { + this.args = { + operator: this.holder, + from: this.holder, + to: this.receiver, + ids: [firstTokenId], + values: [firstTokenValue], + data: '0x', + }; + this.tx = await this.token + .connect(this.args.operator) + .safeBatchTransferFrom(this.args.from, this.args.to, this.args.ids, this.args.values, this.args.data); + }); + + batchTransferWasSuccessful(); + + it('calls onERC1155BatchReceived', async function () { + await expect(this.tx) + .to.emit(this.receiver, 'BatchReceived') + .withArgs(this.holder, this.holder, this.args.ids, this.args.values, this.args.data, anyValue); + }); + }); + + describe('without data (batch of size > 1)', function () { beforeEach(async function () { this.args = { operator: this.holder, @@ -590,7 +668,31 @@ function shouldBehaveLikeERC1155() { }); }); - describe('with data', function () { + describe('with data (batch of size = 1)', function () { + beforeEach(async function () { + this.args = { + operator: this.holder, + from: this.holder, + to: this.receiver, + ids: [firstTokenId], + values: [firstTokenValue], + data: '0xf00dd00d', + }; + this.tx = await this.token + .connect(this.args.operator) + .safeBatchTransferFrom(this.args.from, this.args.to, this.args.ids, this.args.values, this.args.data); + }); + + batchTransferWasSuccessful(); + + it('calls onERC1155BatchReceived', async function () { + await expect(this.tx) + .to.emit(this.receiver, 'BatchReceived') + .withArgs(this.holder, this.holder, this.args.ids, this.args.values, this.args.data, anyValue); + }); + }); + + describe('with data (batch of size > 1)', function () { beforeEach(async function () { this.args = { operator: this.holder, diff --git a/test/token/ERC1155/ERC1155.test.js b/test/token/ERC1155/ERC1155.test.js index 4b0ca0145bc..265cfb17555 100644 --- a/test/token/ERC1155/ERC1155.test.js +++ b/test/token/ERC1155/ERC1155.test.js @@ -2,6 +2,7 @@ const { ethers } = require('hardhat'); const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { RevertType } = require('../../helpers/enums'); const { zip } = require('../../helpers/iterate'); const { shouldBehaveLikeERC1155 } = require('./ERC1155.behavior'); @@ -181,6 +182,42 @@ describe('ERC1155', function () { }); }); + describe('_updateWithAcceptanceCheck', function () { + beforeEach(async function () { + const factory = await ethers.getContractFactory('$ERC1155ReceiverMock'); + this.receiver = await ethers.deployContract('$ERC1155ReceiverMock', [ + factory.interface.getFunction('onERC1155Received').selector, + factory.interface.getFunction('onERC1155BatchReceived').selector, + RevertType.None, + ]); + }); + + it('calls onERC1155Received when only one token is transferred', async function () { + await expect( + this.token.$_updateWithAcceptanceCheck(ethers.ZeroAddress, this.receiver, [tokenId], [mintValue], '0x'), + ).to.emit(this.receiver, 'Received'); + }); + + it('calls onERC1155BatchReceived when only one token is transferred and batch flag is set to true', async function () { + await expect( + this.token.$_updateWithAcceptanceCheck( + ethers.ZeroAddress, + this.receiver, + [tokenId], + [mintValue], + '0x', + ethers.Typed.bool(true), + ), + ).to.emit(this.receiver, 'BatchReceived'); + }); + + it('calls onERC1155BatchReceived when more than one token is transferred', async function () { + await expect( + this.token.$_updateWithAcceptanceCheck(ethers.ZeroAddress, this.receiver, tokenBatchIds, mintValues, '0x'), + ).to.emit(this.receiver, 'BatchReceived'); + }); + }); + describe('_setApprovalForAll', function () { it("reverts when adding an operator over the zero account's tokens", async function () { await expect(this.token.$_setApprovalForAll(ethers.ZeroAddress, this.operator, true))