DatingDapp
First Flight #33
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Incorrect token existence Check in `tokenUri` function

damboy

Summary

The tokenURI function in the SoulboundProfileNFT contract uses ownerOf(tokenId) == address(0) to check if a token exists. However, this check is incorrect because ownerOf(tokenId) reverts if the token does not exist, instead of returning address(0). As a result, calling tokenURI with an invalid tokenId will cause a transaction failure instead of handling the error gracefully.

Vulnerability Details

In the tokenURI function, the following check is used to verify whether a token exists:

if (ownerOf(tokenId) == address(0)) { revert ERC721Metadata__URI_QueryFor_NonExistentToken(); }

However, this approach is flawed because:

  1. ownerOf(tokenId) Does Not Return address(0) for Invalid Tokens

    • The ownerOf function in OpenZeppelin’s ERC721 contract is designed to revert if the token does not exist.

    • It does not return address(0).

    • This means the if-condition will never execute; instead, the function will revert immediately when calling ownerOf with a non-existent tokenId.

  2. How the Issue Manifests

    • If a user queries tokenURI(nonExistentTokenId), the transaction will fail with a revert error from ownerOf, rather than the intended ERC721Metadata__URI_QueryFor_NonExistentToken() error.

    • This makes error handling inconsistent and unexpected.

Proof of Concept (PoC)

Add the test below in the testSoulboundProfileNFT.t.sol and run the command forge test --mt testTokenURI_ShouldRevertForNonExistentToken -vvvv

function testTokenURI_ShouldRevertForNonExistentToken() public {
​
uint256 nonExistentTokenId = 999;
​
vm.expectRevert();
​
soulboundNFT.tokenURI(nonExistentTokenId);
​
}

Your Output should look like the one below:

​
Ran 1 test for test/testSoulboundProfileNFT.t.sol:SoulboundProfileNFTTest
[PASS] testTokenURI_ShouldRevertForNonExistentToken() (gas: 12714)
Traces:
[12714] SoulboundProfileNFTTest::testTokenURI_ShouldRevertForNonExistentToken()
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [2633] SoulboundProfileNFT::tokenURI(999) [staticcall]
│ └─ ← [Revert] ERC721NonexistentToken(999)
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 690.37µs (98.58µs CPU time)
​
Ran 1 test suite in 5.84ms (690.37µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Looking at the Logs, you will see that the contract reverted with ERC721NonexistentToken(999), proving that ownerOf(tokenId) == address(0) does not correctly check token existence.

Impact

  • Unexpected Reverts: Calling tokenURI with an invalid token ID will revert due to ownerOf, rather than executing the intended error-handling logic.

  • Inconsistent Error Handling: The contract's ERC721Metadata__URI_QueryFor_NonExistentToken() error is never actually used, making debugging more difficult.

Tools Used

  • Manual Code Review

  • OpenZeppelin ERC721 Documentation

Recommendations

Use _exists(tokenId) Instead of ownerOf(tokenId) == address(0)

OpenZeppelin provides an _exists(tokenId) function that safely checks if a token exists without reverting. Update tokenURI as follows:

function tokenURI(uint256 tokenId) public view override returns (string memory) {
if (!_exists(tokenId)) {
revert ERC721Metadata__URI_QueryFor_NonExistentToken();
}
string memory profileName = _profiles[tokenId].name;
uint256 profileAge = _profiles[tokenId].age;
string memory imageURI = _profiles[tokenId].profileImage;
​
return string(
abi.encodePacked(
_baseURI(),
Base64.encode(
abi.encodePacked(
'{"name":"', profileName, '", ',
'"description":"A soulbound dating profile NFT.", ',
'"attributes": [{"trait_type": "Age", "value": ', Strings.toString(profileAge), "}], ",
'"image":"', imageURI, '"}'
)
)
)
);
}
​
Updates

Appeal created

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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