ERC1155: Check that owner is not the zero address when adding an operator

[Amxx]

Alternate to #5994

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 86c6c2d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

This change adds input validation to the _setApprovalForAll function in the ERC1155 contract. Specifically, a guard is introduced to reject zero-address owners by reverting with the ERC1155InvalidApprover error. Previously, only zero-address operators were rejected; now both owner and operator zero-address scenarios are explicitly validated. A corresponding test case is added to verify the new validation behavior for zero-address approvers.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding a zero-address validation check for the owner parameter in ERC1155's _setApprovalForAll function.
Description check	✅ Passed	The description is related to the changeset, mentioning it is an alternate to #5994 and indicating tests were added, which aligns with the actual changes made.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/token/ERC1155/ERC1155.test.js (1)

184-190: Consider expanding test coverage.

The test correctly verifies the zero-address owner validation. To enhance completeness, consider:

1. Testing with approved = false to ensure the validation applies regardless of the approval state
2. Clarifying the test description to explicitly mention "zero-address owner/approver" rather than focusing on "adding an operator"

Example expansion:

 describe('_setApprovalForAll', function () {
-  it("reverts when adding an operator over the zero account's tokens", async function () {
+  it('reverts when the owner is the zero address', async function () {
     await expect(this.token.$_setApprovalForAll(ethers.ZeroAddress, this.operator, true))
       .to.be.revertedWithCustomError(this.token, 'ERC1155InvalidApprover')
       .withArgs(ethers.ZeroAddress);
+
+    await expect(this.token.$_setApprovalForAll(ethers.ZeroAddress, this.operator, false))
+      .to.be.revertedWithCustomError(this.token, 'ERC1155InvalidApprover')
+      .withArgs(ethers.ZeroAddress);
   });
 });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d96210 and 86c6c2d.

📒 Files selected for processing (2)

• contracts/token/ERC1155/ERC1155.sol (1 hunks)
• test/token/ERC1155/ERC1155.test.js (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: slither
• GitHub Check: tests-foundry
• GitHub Check: tests-upgradeable
• GitHub Check: coverage
• GitHub Check: tests
• GitHub Check: Redirect rules - solidity-contracts
• GitHub Check: Header rules - solidity-contracts
• GitHub Check: Pages changed - solidity-contracts
• GitHub Check: halmos

🔇 Additional comments (1)

contracts/token/ERC1155/ERC1155.sol (1)

359-361: LGTM! Defensive validation added.

The zero-address owner check correctly prevents invalid state by reverting with the appropriate ERC1155InvalidApprover error. This mirrors the existing operator validation pattern and protects against misuse when the internal function is called directly by derived contracts.