Lack of parameter to check in ERC1155Minimal.sol makes this contract to be not compliant with ERC-1155 standard

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/tokens/ERC1155Minimal.sol#L112-L119
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/tokens/ERC1155Minimal.sol#L163-L170

Vulnerability details

Impact

According to provided documentation, ERC1155Minimal.sol is A minimalist implementation of the ERC1155 token standard without metadata.
Docs straightforwardly mention that ERC1155 used by the protocol are SFPM and factory ERC1155 tokens

However, the current implementation does not comply with EIP-1155. Since the compliance requirement was straightforwardly mentioned as a requirement in the docs - I've evaluated this issue as Medium.

During the previous Code4rena contests, lack of compliance with EIP-1155 was evaluated as Medium too, e.g.:

• https://code4rena.com/reports/2022-07-ens#m-12-erc1155fuse-_transfer-does-not-revert-when-sent-to-the-old-owner

Moreover, during the previous C4 contests, lack of EIP compliance was usually evaluated as High/Medium

• (Lack of EIP-4626 compliance) Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47 - has been evaluated as High
• (Lack of EIP-4524 compliance) HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524 2022-10-holograph-findings#440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) HolographERC721.safeTransferFrom not compliant with EIP-721 2022-10-holograph-findings#203 - has been evaluated as Medium

Lack of compliance may cause unexpected behavior. Other protocols that integrate with contract may incorrectly assume that it's EIP-1155 compliant - especially that documentation states that it's ERC-1155. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-1155, it should fully conform to ERC-1155 standard.

Proof of Concept

According to EIP-1155

safeTransferFrom rules:

MUST revert if _to is the zero address.

[...]

safeBatchTransferFrom rules:

MUST revert if _to is the zero address.

None of these functions verify if _to is the zero address, which implies that contract is not compliant with EIP-1155.

File: ERC1155Minimal.sol

 if (to.code.length != 0) {
            if (
                ERC1155Holder(to).onERC1155Received(msg.sender, from, id, amount, data) !=
                ERC1155Holder.onERC1155Received.selector
            ) {
                revert UnsafeRecipient();
            }
        }

In function safeTransferFrom(), when parameter to is the address zero, to.code.length will return 0, thus we won't enter above conditional branch. The same issue occurs for safeBatchTransferFrom():

File: ERC1155Minimal.sol

        if (to.code.length != 0) {
            if (
                ERC1155Holder(to).onERC1155BatchReceived(msg.sender, from, ids, amounts, data) !=
                ERC1155Holder.onERC1155BatchReceived.selector
            ) {
                revert UnsafeRecipient();
            }
        }

This leads to conclusion that it's possible to safeTransferFrom() or safeBatchTransferFrom() to address(0), even though EIP-1155 straightforwardly states that every address(0) transfer should revert. This means that protocol violates EIP-1155.

Tools Used

Manual code review

Recommended Mitigation Steps

In both safeBatchTransferFrom() and safeTransferFrom() add additional line which will verify if parameter to is not address zero:

require(to != address(0), "Cannot transfer to address(0)");

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #500

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)