Violation of ERC-721 Standard in VerbsToken:tokenURI Implementation

mediumCode4rena

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.

markdown📋
// 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.

patch📋
    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