ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Incorrect assumption about `tokenURI` format causes incorrect translation of token URIs

n4nika

Summary

Currently the bridge makes assumptions about a collection's tokenURI(uint256) implementation that do not necessarily hold, causing L2 to mint NFTs with wrong token_uris.

Vulnerability Details

The bridge currently assumes that for a collections either a baseURI OR a tokenURI is set. More specifically, it assumes the following:

  • if baseURI is set, the tokenURI looks like baseURI::tokenId

  • if no baseURI is set, the tokenURI looks like tokenURI

This holds in many cases but is not necessarily the case. If we look at EIP-721, the following is written about tokenURI:

/// @notice A distinct Uniform Resource Identifier (URI) for a given asset.
/// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
/// 3986. The URI may point to a JSON file that conforms to the "ERC721
/// Metadata JSON Schema".
function tokenURI(uint256 _tokenId) external view returns (string);

Here we see, that the EIP does not specify the URI more than to be RFC 3986 conform.

This means that we can have a NFT collections where we have a baseURI (e.g. http://example.com/) and individual tokenURIs (e.g. token1, token2 ...) where tokenURI() will return a URI with the format baseURI::tokenURI.
If we now call tokenURI(1) we would expect it to return http://example.com/token1 as the URI.

As I pointed out above, the assumption of tokenURI being baseURI::tokenId or tokenURI does not hold here as we have both a common baseURI and also individual tokenURIs.

If we now look at TokenUtil::erc721Metadata, we can see that if our collection has a baseURI (retrieved by _callBaseUri), we return (c.name(), c.symbol(), _baseUri, []), therefore the tokenURIs array is empty.

function erc721Metadata(
address collection,
uint256[] memory tokenIds
)
internal
view
returns (string memory, string memory, string memory, string[] memory)
{
bool supportsMetadata = ERC165Checker.supportsInterface(
collection,
type(IERC721Metadata).interfaceId
);
if (!supportsMetadata) {
return ("", "", "", new string[](0));
}
​
IERC721Metadata c = IERC721Metadata(collection);
// How the URI must be handled.
// if a base URI is already present, we ignore individual URI
// else, each token URI must be bridged and then the owner of the collection
// can decide what to do
(bool success, string memory _baseUri) = _callBaseUri(collection);
if (success) {
return (c.name(), c.symbol(), _baseUri, new string[](0));
}
else {
string[] memory URIs = new string[](tokenIds.length);
for (uint256 i = 0; i < tokenIds.length; i++) {
URIs[i] = c.tokenURI(tokenIds[i]);
}
return (c.name(), c.symbol(), "", URIs);
}
}

Impact

When this request is now received on L2 it is missing crucial information about our individual tokenURIs causing it to mint our NFTs with incorrect URIs.
This means that if we bridge our NFT with the URI http://example.com/token1 from L1 to L2, it will have the URI http://example.com/1 on L2, meaning we cannot retrieve its metadata as its URI is corrupted.

Tools Used

Manual review

Recommended Mitigation

In order to always use the proper metadata of a bridged collection/NFT I would recommend changing TokenUtil::erc721Metadata to take into consideration the case where a baseURI and individual tokenURIs exist.
It is also necessary to change the code on L2 accordingly.

Updates

Lead Judging Commences

n0kto Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!