NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: low

Default ERC721 tokenURI() appends tokenId to collectionImage

Author Revealed upon completion

Description

  • The contract accepts a single collectionImage string in the constructor, stores it, and returns it from _baseURI(). In the current implementation, there is no override of tokenURI(), so the inherited OpenZeppelin ERC721 metadata logic is used for every token.

  • OpenZeppelin’s ERC721 implementation computes tokenURI(tokenId) as string.concat(_baseURI(), tokenId.toString()) whenever the base URI is non-empty. Since this contract’s _baseURI() returns the single image URL stored in collectionImage, every minted token ends up with a URI like collectionImage + "1", collectionImage + "2", etc., instead of a valid per-token metadata URI or the original image URL itself.

  • As a result, if collectionImage is intended to be a direct image URL - as shown by the test suite’s BASE_IMAGE value using a fixed Unsplash image URL - then tokenURI(1) becomes something like https://images.unsplash.com/photo-...1, which is not the original configured asset and is not a proper ERC721 metadata JSON endpoint either. This makes token metadata resolution incorrect for every NFT.

  • Additionally, the contract declares bool public metadataFrozen;, but there is no logic that sets it, checks it, or uses it to control metadata mutability. So the contract both fails to implement correct token metadata and includes a dead metadata-freezing variable that does nothing.

string private collectionImage;
bool public metadataFrozen;
​
constructor(
address _owner,
address _usdc,
string memory _collectionName,
string memory _symbol,
string memory _collectionImage,
uint256 _lockAmount
) ERC721(_collectionName, _symbol) {
owner = _owner;
usdc = IERC20(_usdc);
collectionName = _collectionName;
tokenSymbol = _symbol;
collectionImage = _collectionImage; // @> a single image URL is stored here
lockAmount = _lockAmount;
}
​
function _baseURI() internal view override returns (string memory) {
return collectionImage; // @> returns the raw single image URL as the ERC721 base URI
}
​
// OpenZeppelin ERC721 implementation used by this contract:
//
// function tokenURI(uint256 tokenId) public view virtual returns (string memory) {
// _requireOwned(tokenId);
// string memory baseURI = _baseURI();
// return bytes(baseURI).length > 0 ? string.concat(baseURI, tokenId.toString()) : "";
// }
// @> inherited logic appends tokenId to whatever _baseURI() returns
​

Risk

Likelihood: Low

  • This occurs for every minted token, because every tokenURI(tokenId) call uses the inherited ERC721 concatenation logic and _baseURI() always returns the same collectionImage string. [contracts | Txt], [github.com]

  • This occurs during normal metadata reads by wallets, marketplaces, and indexers, because ERC721 consumers resolve token metadata through tokenURI(tokenId), not through the custom baseURI() helper alone.

Impact: Low

  • All NFTs expose an incorrect token URI, so wallets and marketplaces may fail to load the intended metadata/image or may request a malformed/unintended URL.

  • The declared metadataFrozen variable gives a false impression of metadata immutability controls, even though there is no implemented freezing mechanism.

Proof of Concept

function testDefaultERC721TokenURIAppendsTokenIdToCollectionImage() public {
uint256 tokenId = 1;
​
// Mint token #1 using the existing helper
mintNFTForTesting();
​
string memory returnedBaseURI = nftDealers.baseURI();
string memory returnedTokenURI = nftDealers.tokenURI(tokenId);
string memory expectedTokenURI = string.concat(BASE_IMAGE, "1");
​
console2.log("Configured BASE_IMAGE:");
console2.log(BASE_IMAGE);
​
console2.log("Returned baseURI():");
console2.log(returnedBaseURI);
​
console2.log("Returned tokenURI(1):");
console2.log(returnedTokenURI);
​
console2.log("Expected inherited ERC721 tokenURI result:");
console2.log(expectedTokenURI);
​
console2.log("metadataFrozen flag:");
console2.log(nftDealers.metadataFrozen() ? "true" : "false");
​
// Sanity: the contract stores and returns the original collection image as baseURI.
assertEq(returnedBaseURI, BASE_IMAGE, "sanity: baseURI should equal configured collection image");
​
// Core bug signal:
// tokenURI() is NOT the configured image URL; tokenId is appended by the inherited ERC721 logic.
assertEq(
returnedTokenURI,
expectedTokenURI,
"BUG: tokenURI appends tokenId to the configured collection image"
);
​
assertTrue(
keccak256(bytes(returnedTokenURI)) != keccak256(bytes(BASE_IMAGE)),
"BUG: tokenURI should not differ from the configured collection image in this metadata model"
);
}

Recommended Mitigation

function _baseURI() internal view override returns (string memory) {
return collectionImage;
}
​
+function tokenURI(uint256 tokenId) public view override returns (string memory) {
+ ownerOf(tokenId); // reverts if token does not exist
+ return collectionImage;
+}

Support

FAQs

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

Give us feedback!