Skip to content

Commit 2efe235

Browse files
Amxxjames-toussaintcoderabbitai[bot]gonzaotc
authored
Call checkOnERC1155BatchReceived when transfering a batch of length 1 (#6170)
Co-authored-by: James Toussaint <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Gonzalo Othacehe <[email protected]>
1 parent 353f564 commit 2efe235

File tree

5 files changed

+182
-14
lines changed

5 files changed

+182
-14
lines changed

‎.changeset/vast-worlds-pull.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': minor
3+
---
4+
5+
`ERC1155`: Call `IERC1155Receiver.onERC1155BatchReceived` when performing a batch transfers with exactly one id/value in the batch.

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
### Breaking changes
44

5+
- `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))
56
- `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))
67
- `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))
78
- `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))

‎contracts/token/ERC1155/ERC1155.sol‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -178,23 +178,46 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
178178
* IMPORTANT: Overriding this function is discouraged because it poses a reentrancy risk from the receiver. So any
179179
* update to the contract state after this function would break the check-effect-interaction pattern. Consider
180180
* overriding {_update} instead.
181+
*
182+
* NOTE: This version is kept for backward compatibility. We recommend calling the alternative version with a boolean
183+
* flag in order to achieve better control over which hook to call.
181184
*/
182185
function _updateWithAcceptanceCheck(
183186
address from,
184187
address to,
185188
uint256[] memory ids,
186189
uint256[] memory values,
187190
bytes memory data
191+
) internal virtual {
192+
_updateWithAcceptanceCheck(from, to, ids, values, data, ids.length != 1);
193+
}
194+
195+
/**
196+
* @dev Version of {_update} that performs the token acceptance check by calling
197+
* {IERC1155Receiver-onERC1155Received} or {IERC1155Receiver-onERC1155BatchReceived} on the receiver address if it
198+
* contains code (eg. is a smart contract at the moment of execution).
199+
*
200+
* IMPORTANT: Overriding this function is discouraged because it poses a reentrancy risk from the receiver. So any
201+
* update to the contract state after this function would break the check-effect-interaction pattern. Consider
202+
* overriding {_update} instead.
203+
*/
204+
function _updateWithAcceptanceCheck(
205+
address from,
206+
address to,
207+
uint256[] memory ids,
208+
uint256[] memory values,
209+
bytes memory data,
210+
bool batch
188211
) internal virtual {
189212
_update(from, to, ids, values);
190213
if (to != address(0)) {
191214
address operator = _msgSender();
192-
if (ids.length == 1) {
215+
if (batch) {
216+
ERC1155Utils.checkOnERC1155BatchReceived(operator, from, to, ids, values, data);
217+
} else {
193218
uint256 id = ids.unsafeMemoryAccess(0);
194219
uint256 value = values.unsafeMemoryAccess(0);
195220
ERC1155Utils.checkOnERC1155Received(operator, from, to, id, value, data);
196-
} else {
197-
ERC1155Utils.checkOnERC1155BatchReceived(operator, from, to, ids, values, data);
198221
}
199222
}
200223
}
@@ -219,7 +242,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
219242
revert ERC1155InvalidSender(address(0));
220243
}
221244
(uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value);
222-
_updateWithAcceptanceCheck(from, to, ids, values, data);
245+
_updateWithAcceptanceCheck(from, to, ids, values, data, false);
223246
}
224247

225248
/**
@@ -246,7 +269,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
246269
if (from == address(0)) {
247270
revert ERC1155InvalidSender(address(0));
248271
}
249-
_updateWithAcceptanceCheck(from, to, ids, values, data);
272+
_updateWithAcceptanceCheck(from, to, ids, values, data, true);
250273
}
251274

252275
/**
@@ -288,7 +311,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
288311
revert ERC1155InvalidReceiver(address(0));
289312
}
290313
(uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value);
291-
_updateWithAcceptanceCheck(address(0), to, ids, values, data);
314+
_updateWithAcceptanceCheck(address(0), to, ids, values, data, false);
292315
}
293316

294317
/**
@@ -307,7 +330,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
307330
if (to == address(0)) {
308331
revert ERC1155InvalidReceiver(address(0));
309332
}
310-
_updateWithAcceptanceCheck(address(0), to, ids, values, data);
333+
_updateWithAcceptanceCheck(address(0), to, ids, values, data, true);
311334
}
312335

313336
/**
@@ -325,7 +348,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
325348
revert ERC1155InvalidSender(address(0));
326349
}
327350
(uint256[] memory ids, uint256[] memory values) = _asSingletonArrays(id, value);
328-
_updateWithAcceptanceCheck(from, address(0), ids, values, "");
351+
_updateWithAcceptanceCheck(from, address(0), ids, values, "", false);
329352
}
330353

331354
/**
@@ -343,7 +366,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER
343366
if (from == address(0)) {
344367
revert ERC1155InvalidSender(address(0));
345368
}
346-
_updateWithAcceptanceCheck(from, address(0), ids, values, "");
369+
_updateWithAcceptanceCheck(from, address(0), ids, values, "", true);
347370
}
348371

349372
/**

‎test/token/ERC1155/ERC1155.behavior.js‎

Lines changed: 107 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,24 @@ function shouldBehaveLikeERC1155() {
147147
.withArgs(this.holder, firstTokenValue, firstTokenValue + 1n, firstTokenId);
148148
});
149149

150+
it('reverts when transferring from zero address', async function () {
151+
await expect(
152+
this.token
153+
.connect(this.holder)
154+
.safeTransferFrom(ethers.ZeroAddress, this.holder, firstTokenId, firstTokenValue, '0x'),
155+
)
156+
.to.be.revertedWithCustomError(this.token, 'ERC1155MissingApprovalForAll')
157+
.withArgs(this.holder, ethers.ZeroAddress);
158+
159+
await expect(
160+
this.token
161+
.connect(this.holder)
162+
.$_safeTransferFrom(ethers.ZeroAddress, this.holder, firstTokenId, firstTokenValue, '0x'),
163+
)
164+
.to.be.revertedWithCustomError(this.token, 'ERC1155InvalidSender')
165+
.withArgs(ethers.ZeroAddress);
166+
});
167+
150168
it('reverts when transferring to zero address', async function () {
151169
await expect(
152170
this.token
@@ -442,6 +460,36 @@ function shouldBehaveLikeERC1155() {
442460
.withArgs(ids2.length, tokenValues2.length);
443461
});
444462

463+
it('reverts when transferring from zero address', async function () {
464+
await expect(
465+
this.token
466+
.connect(this.holder)
467+
.safeBatchTransferFrom(
468+
ethers.ZeroAddress,
469+
this.holder,
470+
[firstTokenId, secondTokenId],
471+
[firstTokenValue, secondTokenValue],
472+
'0x',
473+
),
474+
)
475+
.to.be.revertedWithCustomError(this.token, 'ERC1155MissingApprovalForAll')
476+
.withArgs(this.holder, ethers.ZeroAddress);
477+
478+
await expect(
479+
this.token
480+
.connect(this.holder)
481+
.$_safeBatchTransferFrom(
482+
ethers.ZeroAddress,
483+
this.holder,
484+
[firstTokenId, secondTokenId],
485+
[firstTokenValue, secondTokenValue],
486+
'0x',
487+
),
488+
)
489+
.to.be.revertedWithCustomError(this.token, 'ERC1155InvalidSender')
490+
.withArgs(ethers.ZeroAddress);
491+
});
492+
445493
it('reverts when transferring to zero address', async function () {
446494
await expect(
447495
this.token
@@ -484,9 +532,15 @@ function shouldBehaveLikeERC1155() {
484532
});
485533

486534
it('emits a TransferBatch log', async function () {
487-
await expect(this.tx)
488-
.to.emit(this.token, 'TransferBatch')
489-
.withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids, this.args.values);
535+
if (this.args.ids.length == 1) {
536+
await expect(this.tx)
537+
.to.emit(this.token, 'TransferSingle')
538+
.withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids[0], this.args.values[0]);
539+
} else {
540+
await expect(this.tx)
541+
.to.emit(this.token, 'TransferBatch')
542+
.withArgs(this.args.operator, this.args.from, this.args.to, this.args.ids, this.args.values);
543+
}
490544
});
491545
}
492546

@@ -566,7 +620,31 @@ function shouldBehaveLikeERC1155() {
566620
]);
567621
});
568622

569-
describe('without data', function () {
623+
describe('without data (batch of size = 1)', function () {
624+
beforeEach(async function () {
625+
this.args = {
626+
operator: this.holder,
627+
from: this.holder,
628+
to: this.receiver,
629+
ids: [firstTokenId],
630+
values: [firstTokenValue],
631+
data: '0x',
632+
};
633+
this.tx = await this.token
634+
.connect(this.args.operator)
635+
.safeBatchTransferFrom(this.args.from, this.args.to, this.args.ids, this.args.values, this.args.data);
636+
});
637+
638+
batchTransferWasSuccessful();
639+
640+
it('calls onERC1155BatchReceived', async function () {
641+
await expect(this.tx)
642+
.to.emit(this.receiver, 'BatchReceived')
643+
.withArgs(this.holder, this.holder, this.args.ids, this.args.values, this.args.data, anyValue);
644+
});
645+
});
646+
647+
describe('without data (batch of size > 1)', function () {
570648
beforeEach(async function () {
571649
this.args = {
572650
operator: this.holder,
@@ -590,7 +668,31 @@ function shouldBehaveLikeERC1155() {
590668
});
591669
});
592670

593-
describe('with data', function () {
671+
describe('with data (batch of size = 1)', function () {
672+
beforeEach(async function () {
673+
this.args = {
674+
operator: this.holder,
675+
from: this.holder,
676+
to: this.receiver,
677+
ids: [firstTokenId],
678+
values: [firstTokenValue],
679+
data: '0xf00dd00d',
680+
};
681+
this.tx = await this.token
682+
.connect(this.args.operator)
683+
.safeBatchTransferFrom(this.args.from, this.args.to, this.args.ids, this.args.values, this.args.data);
684+
});
685+
686+
batchTransferWasSuccessful();
687+
688+
it('calls onERC1155BatchReceived', async function () {
689+
await expect(this.tx)
690+
.to.emit(this.receiver, 'BatchReceived')
691+
.withArgs(this.holder, this.holder, this.args.ids, this.args.values, this.args.data, anyValue);
692+
});
693+
});
694+
695+
describe('with data (batch of size > 1)', function () {
594696
beforeEach(async function () {
595697
this.args = {
596698
operator: this.holder,

‎test/token/ERC1155/ERC1155.test.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ const { ethers } = require('hardhat');
22
const { expect } = require('chai');
33
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
44

5+
const { RevertType } = require('../../helpers/enums');
56
const { zip } = require('../../helpers/iterate');
67
const { shouldBehaveLikeERC1155 } = require('./ERC1155.behavior');
78

@@ -181,6 +182,42 @@ describe('ERC1155', function () {
181182
});
182183
});
183184

185+
describe('_updateWithAcceptanceCheck', function () {
186+
beforeEach(async function () {
187+
const factory = await ethers.getContractFactory('$ERC1155ReceiverMock');
188+
this.receiver = await ethers.deployContract('$ERC1155ReceiverMock', [
189+
factory.interface.getFunction('onERC1155Received').selector,
190+
factory.interface.getFunction('onERC1155BatchReceived').selector,
191+
RevertType.None,
192+
]);
193+
});
194+
195+
it('calls onERC1155Received when only one token is transferred', async function () {
196+
await expect(
197+
this.token.$_updateWithAcceptanceCheck(ethers.ZeroAddress, this.receiver, [tokenId], [mintValue], '0x'),
198+
).to.emit(this.receiver, 'Received');
199+
});
200+
201+
it('calls onERC1155BatchReceived when only one token is transferred and batch flag is set to true', async function () {
202+
await expect(
203+
this.token.$_updateWithAcceptanceCheck(
204+
ethers.ZeroAddress,
205+
this.receiver,
206+
[tokenId],
207+
[mintValue],
208+
'0x',
209+
ethers.Typed.bool(true),
210+
),
211+
).to.emit(this.receiver, 'BatchReceived');
212+
});
213+
214+
it('calls onERC1155BatchReceived when more than one token is transferred', async function () {
215+
await expect(
216+
this.token.$_updateWithAcceptanceCheck(ethers.ZeroAddress, this.receiver, tokenBatchIds, mintValues, '0x'),
217+
).to.emit(this.receiver, 'BatchReceived');
218+
});
219+
});
220+
184221
describe('_setApprovalForAll', function () {
185222
it("reverts when adding an operator over the zero account's tokens", async function () {
186223
await expect(this.token.$_setApprovalForAll(ethers.ZeroAddress, this.operator, true))

0 commit comments

Comments
 (0)