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

Starknet bridge contract does not check if the collection supports IERC721Metadata interface, so the ones that do not implement it will not be able to bridge NFTs

draiakoo

Summary

Starknet bridge contract does not check if the collection supports IERC721Metadata interface, so the ones that do not implement it will not be able to bridge NFTs

Vulnerability Details

In the Ethereum bridge, when a user deposits an NFT it is needed to get the name, symbol, baseURI and tokenURIs from the specific collection. To do that, it first checks if the contract collection supports this interface.

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);
}
}

As we can see, if the collection does not support this interface it is returned all fields empty. Otherwise, the functions to retrieve all the informations are called.

However, in the Starknet bridge, no interface is checked and the functions to retrieve all these informations are called directly.

fn erc721_metadata(
contract_address: ContractAddress,
token_ids: Option<Span<u256>>
) -> Option<ERC721Metadata> {
let erc721 = IERC721Dispatcher { contract_address };
​
...
​
Option::Some(
ERC721Metadata {
name: erc721.name(),
symbol: erc721.symbol(),
base_uri: "",
uris
}
)
}

This missing check for the interface will make NFT collections that do not implement the IERC721Metadata interface unable to be bridged because these functions will not exist and the transaction will revert.

Note that the IERC721Metadata is an OPTIONAL interface as stated in the EIP:

The metadata extension is OPTIONAL for ERC-721 smart contracts (see “caveats”, below). This allows your smart contract to be interrogated for its name and for details about the assets which your NFTs represent.

That means that a collection will be ERC721 compliant even though it does not implement the IERC721Metadata interface. But will be unable to work with the Starknet bridge.

Impact

Medium, collections that do not implement this interface will be unable to be bridge from Starknet to Ethereum

Tools Used

Manual review

Recommendations

Check if the collection supports the interface before calling these methods just as the Ethereum bridge does

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

invalid-NFT-minted-without-baseURI-URIs-or-bridge-with-no-metadata

URI is not lost on the origin chain and it can be modified with `ERC721UriImpl`. As explained in the TODO  below, that’s a design choice and it will be implemented as a future feature. https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L206 `ERC721Bridgable` is out of scope.

Appeal created

haxatron Auditor
about 1 year ago
n0kto Lead Judge
about 1 year ago
n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-L2-ERC721-without-metadata-extension-wont-work

Impact: Medium/High, ERC721 tokens won’t work without the metadata extension (name + symbol), but no tokens are loss. Likelyhood: Low, All tokens not implementing name and symbol will be impacted. This should be pretty rare because the two main "libraries" to create an ERC721, have those function in their main contract and not in the metadata extension. OZ since the version 3.0.0 and Solmate since their first production release. But as anyone can also implement their own ERC721 following the EIP, it deserves a medium.

Support

FAQs

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

Give us feedback!