OpenZeppelin
[Bug] `ERC1155._updateWithAcceptanceCheck` ignores `data` considerations
ERC1155 incorrectly routes a safeBatchTransferFrom (batched) to checkOnERC1155Received (not-batched) when .length == 1
💻 Environment
📝 Details
if (ids.length == 1) {
should be
~~if (ids.length == 1 && data.length == 0) {~~
_updateWithAcceptanceCheck() should pass a boolean whether it was invoked via single or batch and route accordingly.
As-is, data payloads must be special cased for .length == 1 and safeBatchTransferFrom should not be called with .length == 1.
Hello @adraffy.
I'd be curious to understand in what cases this causes an issue.
If you ask me, IERC1155Receiver having two different function is a bad design, and onERC1155BatchReceived should be the only receiver. That being said, I would expect MOST implementations of IERC1155Receiver to implement the batch receiver as a loop that executes the single logic N times. I would also expect ALL implementations to consider the "getting a batch of 1" case to be equivalent to "getting one type of token".
In a batch, the data parameter is singular and opaque, but it's context is an array of tokens.
The current code assumes the data payload is authorization-like and independent of the number of tokens transferred, and not specific to the batch call, eg. an array of payloads.
If I have a function that dynamically constructs an array of ids, amounts, etc., likely I would construct an an array of data, and then encode it into the batch data payload. When .length is 0 or 2+, this will call the batch handler as expected, which will decode the encoded array, and handle each token. When .length == 1, the underlying 1155 will instead pass that payload to the single receiver callback.
This means onERC1155Received will receive data payloads from safeTransferFrom for a single token AND safeBatchTransferFrom for an array of 1 token, in the situation where the data is encoded specific to type of transfer.
When data is present, it seems incorrect to for safeBatchTransferFrom to invoke onERC1155Received.
I think @adraffy's point is that the data argument is encoded/formatted according to the type of transfer being performed. Even if the transfer is a batch of 1, the data would likely be encoded as an array (with a single value), so calling checkOnERC1155Received instead of checkOnERC1155BatchReceived will cause the receiver to not handle the data argument properly. Is that correct?
I still agree with @Amxx that having 2 different functions for reception is a bad design and the ERC specifies that calling any of the receiving functions (see scenario 6 of the ERC).
Correct.
I also agree having multiple receiver callbacks is a bad design, but fixing it is a breaking change.
Whereas safeBatchTransferFrom passing it's data (when non-empty) to onERC1155Received is simply incorrect.
In 4.0, ERC1155 worked correctly.
Let me see if I can find when the bug was added.
The bug was introduced during v4.9 → v5.0.
Proof of concept: https://github.com/adraffy/immunefi-oz-1155/blob/default/test/Test.sol
I agree it's a breaking change in a strict sense, but this is expected on a major bump to the library and it was documented in the changelog. Although it can have unexpected consequences on the receiver, I would argue that given how ERC-1155 is specified, any receiver should consider both cases regardless of OpenZeppelin's implementation (i.e. always call onERC1155BatchReceived for batches regardless of the size or onERC1155Received for batches of 1).
Whereas safeBatchTransferFrom passing it's data (when non-empty) to onERC1155Received is simply incorrect.
Here I disagree, the ERC doesn't enforce one or the other, so clasifying this behavior as incorrect seems imprecise since anyone else could open an issue stating that their receiver can't handle batches of 1.
Here I disagree, the ERC doesn't enforce one or the other, so clasifying this behavior as incorrect seems imprecise since anyone else could open an issue stating that their receiver can't handle batches of 1.
EIP-1155 clearly says:
An ERC1155-compliant smart contract MUST call this function on the token recipient contract, at the end of a
safeBatchTransferFromafter the balances have been updated.
And this was the OpenZeppelin behavior prior to 5.0.
The changelog comment above only refers to the event.
I submitted a bug report but it was closed by @ernestognw falsely claiming that the change log describes the issue.
The referenced change only mentions the event behavior.
The change does not discuss that in 5.0, the ERC-1155 violates the EIP-1155 spec.