Root + Impact

Description

• The tokenURI() function normally returns the metadata URI for a given token ID, assuming the token exists. The standard OpenZeppelin ERC721 contract’s ownerOf(tokenId) reverts automatically when called with a non-existent token ID.

• The problem is that the code redundantly checks if ownerOf(tokenId) == address(0) to detect non-existent tokens. However, ownerOf() already reverts for invalid tokens, so this check is unreachable and unnecessary, adding complexity without benefit.

Risk

Likelihood:

• This redundant check will never evaluate to true because ownerOf(tokenId) reverts first on non-existent tokens.

• The function may be slightly less clear or efficient due to unreachable code.

Impact:

• Minor gas cost increase due to unnecessary conditional check.

• Possible confusion for developers reading the code, potentially leading to misunderstandings of how ownerOf behaves.

Proof of Concept

Calling tokenURI with a non-existent tokenId will revert from ownerOf call, never reaching the custom revert in the code.

Recommended Mitigation

Replace the unnecessary ownerOf(tokenId) == address(0) check with an explicit call to the _exists(tokenId) function. This function returns true if the token has been minted and exists, otherwise false. This makes the intent clearer and prevents relying on ownerOf’s internal revert behavior. It also allows you to control the revert error message precisely.