ArkProject: NFT Bridge

Vulnerability Details

When retrieving ERC721 collection metadata values (name, symbol, token_uri), we are retrieving their returned data to be ByteArray. Since these functions will return string values we need to understand what is the data type of string value in Cairo.

Cairo is a low-level programming language, it does not support strings, and strings are represented using either felt252 or ByteArray.

If the string length is smaller than 31 length it can be represented in only one felt252.

First we need to understand why there are two types for retrieving strings.

The original type was normal felt252, but on March 2024 Starknet introduced ByteArray type.

This means that all NFT collections (ERC721 tokens) are created from the launching of the Starknet blockchain till the type of supporting ByteArray having an interface that returns the name, symbol, and token_uri as felt252.

OpenZepeplin starknet contract versions from the beginning to v0.9.0 use felt252 for their ERC721 tokens.

OZ::0.9.0::erc721.cairo#L219-L221

And from v0.10.0 to v0.15.0 they use ByteArray. The protocol uses v0.11.0, which assigns the returned value to ByteArray.

OZ::0.11.0::erc721.cairo#L221-L223

The Starknet launched November 2021 or February 2022, and ByteArray was supported on March 2024. so all ERC721 tokens created in these 2 years have an interface that returns felt252 for name, symbol, and token_uri.

The protocol is not handling such a case and deals with NFTs on layer2 as they return ByteArray when calling name, symbol, which is totally incorrect, as the majority of NFTs were created before supporting ByteArray so dealing with the returned value when calling <collection>.name() will only work for ERC721 tokens that was created after supporting ByteArray, and use it.

When making calls there are two types of calls in starknet:

1. low-level call: similar to .call in solidity.

2. High-level calls: interface calls.

In solidity the returned data is in Bytes whatever its type was, the same is in Starknet when doing low-level call the returned type is Span what ever the type was.

We are handling retrieving token_uri correctly, where we are doing an internal call when getting them, which result in a return value as Span<felt252>

collection_manager.cairo#L107-L123

Since the returned value is felt252, we will convert it into ByteArray without problems and there is a custom implementation for try_into than handles converting felt252 into ByteArray correctly in byte_array_extra.

The issue is that when getting token name and symbol we are retrieving them with High-level call.

collection_manager.cairo#L69-L70

The interface we are defining ERC721Metadata declares string values as ByteArray we explained why uris will get handled correctly, and for base_uri it is written manually, but for name() and symbol() we are calling them directly and expect the returned value to be of type ByteArray.

erc721 is the collection address we are going to Bridge the token from it from L2 to L1, and since not all NFT collections on L2, but actually the majority of NFT collections on L2 return felt252, making erc721.name() will result in an exception because of incorrect assigning. the execution will stop even we have an Option return value because this is an execution error that occurred before reaching the end of the function.

Proof of Concept

Doing a POC for such an issue is complicated, but to make it achievable we separated it into two parts:

1. In the Starknet development environment make another erc721_collection that returns name(), symbol(), token_uri() as strings. Achieving this needs a lot of modifications to erc721_collection.cairo file so we made another file erc721_collection_2.cairo and make it as an NFT collection that returns felt252.

2. Add the following test script in the last of apps/blockchain/starknet/src/tests/bridge_t.cairo.

Run the following command.

Output

NOTE: To not make the report too large, we mentioned in point 1 what we need in order for the POC to work, all you have to do is to deploy an erc721_collection that returns felt252 for name(), and we preferred to leave this to you as mention how to make this needs a lot of modifications. And for the POC, you need to make sure that you are deploying the correct address, our POC works when there are two contracts erc721_collection and erc721_collection_2 and the 2 version is the one that returns felt252, you need to import these contract and make sure to add them in order to get them in artificats and not receive path errors. And ofc having the sponsor with you when setting it up is the best

Impact

• The majourity of NFTs on L2, which returns felt252 when calling name() or symbol() can't get briged.

Tools Used

Manual review + Starknet Foundry

Recommendations

reterive erc721.name() and erc721.symbol() using low-level to get the result as Span<felt252> then convert them into ByteArray.