Violation of ERC-721 Standard in VerbsToken:tokenURI Implementation

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol#L193

Vulnerability details

Impact

The VerbsToken contract deviates from the ERC-721 standard, specifically in the tokenURI implementation. According to the standard, the tokenURI method must revert if a non-existent tokenId is passed. In the VerbsToken contract, this requirement was overlooked, leading to a violation of the EIP-721 specification and breaking the invariants declared in the protocol's README.

Proof of Concept

The responsibility for checking whether a token exists may be argued to be placed on the descriptor. However, the core VerbsToken contract, which is expected to adhere to the invariant stated in the Protocol's README, does not follow the specification.

// File: README.md
414:## EIP conformity
415:
416:- [VerbsToken](https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol): Should comply with `ERC721`

Note: the original NounsToken contract, which VerbsToken was forked from, did implement the tokenURI function properly.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to strictly adopt the implementation from the original NounsToken contract to ensure compliance with the ERC-721 standard.

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
+        require(_exists(tokenId));
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

References

1. EIP-721 Standard
2. Code 423n4 Finding - Caviar
3. Code 423n4 Finding - OpenDollar
4. NounsToken Contract Implementation

Assessed type

ERC721

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #110

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[osmanozdemir1]

Hi @MarioPoneder

I assume this issue is accepted based on historical C4 judging even though there is no real impact and it is just a view function. I would like to point out that just a few weeks ago a similar issue was judged as QA due not having a real impact even though it breaks two different "MUST" rules of the EIP. Ref: https://github.com/code-423n4/2023-11-panoptic-findings/issues/473

I acknowledge it is a thin line since there is not a certain rule in the org repo regarding EIPs. Maybe C4 should have a rule for EIP's (at least for the "MUST" rules) but of course here is not the place. I just wanted to point out downgraded issue.

Thanks.

[MarioPoneder]

Thank you for your comment!

I agree from a personal point of view.

However, I felt obliged to award with Medium severity due to precedent EIP-721 tokenUri cases (see https://github.com/code-423n4/2023-12-revolutionprotocol-findings/issues/511#issuecomment-1883512625, one judged by Alex).

This should be discussed during the next SC round.