Snowman Merkle Airdrop
First Flight #42
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Ineffective Token Existence Check in `tokenURI` Function

evmninja

Summary: The tokenURI function contains an incorrect token existence check that can never be triggered, resulting in unreachable code.

Description: The Snowman contract includes a check in the tokenURI function that attempts to verify whether a token exists before returning its URI. However, this check is implemented incorrectly. The code compares the result of ownerOf(tokenId) against address(0) to determine if the token exists.

The fundamental issue is that OpenZeppelin's ERC721 implementation never returns address(0) from the ownerOf function. Instead, when queried for a non-existent token, the ownerOf function reverts with a custom error. As a result, the condition ownerOf(tokenId) == address(0) can never be true, and the custom error ERC721Metadata__URI_QueryFor_NonExistentToken() is never thrown.

The vulnerability cannot be directly exploited to cause harm, but it represents a misunderstanding of how the underlying ERC721 contract works and creates misleading code that suggests functionality that does not actually work.

Step-by-step Analysis:

  1. The tokenURI function attempts to check token existence by comparing ownerOf(tokenId) to address(0).

  2. However, in OpenZeppelin's ERC721 implementation, ownerOf reverts with a custom error when called for a non-existent token.

  3. The execution never reaches the comparison because it reverts earlier in the call stack.

  4. Thus, the custom error ERC721Metadata__URI_QueryFor_NonExistentToken() is dead code that can never be triggered.

  5. The function has redundant logic since the ownerOf function already handles the token existence check.

Severity Classification:

  • Impact: Medium - While not directly exploitable, it indicates a misunderstanding of the underlying contract's behaviour. This could lead to confusion for developers and auditors and potentially mask other issues.

  • Likelihood: High - This issue is present in all calls to tokenURI with non-existent tokens, but the impact is limited to code quality rather than functionality.

File Name: src/Snowman.sol

Code:

function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
if (ownerOf(tokenId) == address(0)) {
revert ERC721Metadata__URI_QueryFor_NonExistentToken();
}
string memory imageURI = s_SnowmanSvgUri;
​
return string(
abi.encodePacked(
_baseURI(),
Base64.encode(
abi.encodePacked(
'{"name":"',
name(),
'", "description":"Snowman for everyone!!!", ',
'"attributes": [{"trait_type": "freezing", "value": 100}], "image":"',
imageURI,
'"}'
)
)
)
);
}

Recommendation:
Either remove the redundant check entirely (since OpenZeppelin's ownerOf already handles this) or replace it with a check that actually works, such as using the _exists function.

Recommended Code Fix:
Option 1: Remove the redundant check (preferred):

function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
// The OpenZeppelin ownerOf function will revert if the token doesn't exist
string memory imageURI = s_SnowmanSvgUri;
​
return string(
abi.encodePacked(
_baseURI(),
Base64.encode(
abi.encodePacked(
'{"name":"',
name(),
'", "description":"Snowman for everyone!!!", ',
'"attributes": [{"trait_type": "freezing", "value": 100}], "image":"',
imageURI,
'"}'
)
)
)
);
}

Option 2: Use _exists for an explicit check:

function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
if (!_exists(tokenId)) {
revert ERC721Metadata__URI_QueryFor_NonExistentToken();
}
string memory imageURI = s_SnowmanSvgUri;
​
// ... rest of function remains the same
}

Either fix would ensure the code accurately reflects the intended functionality and removes the dead code.

Proof of Concept:

function test_POC_IncorrectTokenExistenceCheck() public {
// Mint a token to test valid case
snowman.mintSnowman(address(this), 1);
// This works fine - token exists
string memory validURI = snowman.tokenURI(0);
// For a non-existent token, OpenZeppelin's ownerOf will revert
// The custom error check is never reached
vm.expectRevert(); // OpenZeppelin error, not the custom one
snowman.tokenURI(999);
// Prove that ownerOf reverts instead of returning address(0)
vm.expectRevert();
snowman.ownerOf(999);
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.