search

code-423n4 / 2024-04-panoptic-findings

7 stars 3 forks source link

SemiFungiblePositionManager/ERC1155Minimal is not EIP1155 compliant #500

Closed c4-bot-3 closed 3 months ago

c4-bot-3 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L94-L120 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L130-L171

Vulnerability details

Impact

SemiFungiblePositionManager/ERC1155Minimal is not fully EIP1155 compliant.

Bug Description

There are two behaviours that does not comply with EIP1155: https://eips.ethereum.org/EIPS/eip-1155.

  1. During safeBatchTransfer and safeBatchTransferFrom, it does not revert if the recipient is zero address.

  2. During safeBatchTransferFrom, it does not revert if _ids is not the same length as _values.

Quote from EIP1155:

safeTransferFrom rules:

  • MUST revert if _to is the zero address.

safeBatchTransferFrom rules:

  • MUST revert if _to is the zero address.
  • MUST revert if length of _ids is not the same as length of _values.

Proof of Concept

See quote from EIP1155.

Tools Used

Manual review

Recommended Mitigation Steps

Add corresponding checks.

Assessed type

Other

c4-judge commented 4 months ago

Picodes marked the issue as primary issue

dyedm1 commented 4 months ago

True, that's why we didn't put it on the compliance checklist

Picodes commented 4 months ago

@dyedm1 so you're saying that it never was your intention to have full compliance to the standard?

c4-judge commented 4 months ago

Picodes changed the severity to QA (Quality Assurance)

dyedm1 commented 4 months ago

Correct. The ERC1155 standard is a little too opinionated IMO