sherlock-audit / 2024-04-titles-judging

10 stars 7 forks source link

xiaoming90 - EIP 712 is not implemented for the `Edition` contract. #282

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 5 months ago

xiaoming90

medium

EIP 712 is not implemented for the Edition contract.

Summary

EIP 712 is not implemented for the Edition contract.

Vulnerability Detail

The Contest's README stated that EIP 712 must be strictly implemented for the Edition contract. Thus, in the context of this audit contest, any non-compliance is considered a valid Medium issue. Note that the Contest's README supersedes Sherlock's judging rules per Sherlock's Hierarchy of truth.

Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification?

strict implementation of EIPs 1271 (Graph), 712 (Graph, Edition), 2981 (Edition), 1155 (Edition)

However, when reviewing the Edition contract, it was observed that EIP 712 is not implemented for the Edition contract. Thus, breaking the above requirement.

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L36

File: Edition.sol
34: /// @title Edition
35: /// @notice An ERC1155 contract representing a collection of related works. Each work is represented by a token ID.
36: contract Edition is IEdition, ERC1155, ERC2981, Initializable, OwnableRoles {
37:     using SafeTransferLib for address;
..SNIP..

Impact

EIP 712 is not implemented for the Edition contract.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L36

Tool used

Manual Review

Recommendation

Consider implementing EIP 712 for the Edition contract.

pqseags commented 5 months ago

This seems valid based on the Readme

Hash01011122 commented 4 months ago

Seems borderline low/medium

0x73696d616f commented 4 months ago

Escalate

Should be low as it does not have any impact. EIP712 issues are valid when it causes problems due to not being able to correctly sign signatures, but this is not the case here.

sherlock-admin3 commented 4 months ago

Escalate

Should be low as it does not have any impact. EIP712 issues are valid when it causes problems due to not being able to correctly sign signatures, but this is not the case here.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

realfugazzi commented 4 months ago

Agree with invalidation reason, looks like a feature request.

cducrest commented 4 months ago

EIP712 does not define an interface contract or anything in the like, it is about hashing and signing typed structured data. This issue does not indicate any non-compliance with EIP712. It seems to be pointed that the contract does not inherit from an IEIP712 interface or the like, which is not a problem considering EIP712.

See https://eips.ethereum.org/EIPS/eip-712. See also Solady's implementation of EIP712 which defines only one public function eip712Domain() defined in EIP5267 and not EIP712: https://github.com/vectorized/solady/blob/main/src/utils/EIP712.sol https://eips.ethereum.org/EIPS/eip-5267

WangSecurity commented 4 months ago

Agree with the escalation. Planning to accept it and invalidate the issue, unless someone shows and proves how it effects external integrations.

Hash01011122 commented 4 months ago

Agree with the escalation this is indeed low

Evert0x commented 4 months ago

Result: Invalid Unique

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: