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

L2 ERC721 tokens that do not implement the token_uri function cannot be bridged to L1

crunter

Summary

Unlike local testing, Starknet live blockchains (mainnet or testnet) cannot handle the call_contract_syscall failure when trying to bridge a particular L2 ERC721 token. In such case the process reverts, making it impossible to handle the error when the token_uri function does not exist.

Vulnerability Details

As noted in the cairo book, the starknet docs and discussed in the Openzeppelin/cairo-contracts open issue #904, an internal call cannot return Err(_), and if call_contract_syscall fails, the entire transaction will be reverted.

With this in mind, the match structure used in collection_manager::token_uri_from_contract_call function to obtain the token URI will cause the process to revert when trying to bridge a token with a tokenURI function instead token_uri.

fn token_uri_from_contract_call(
collection_address: ContractAddress,
token_id: u256,
) -> Option<ByteArray> {
// TODO: add the interface detection when the standard is out.
​
@> let token_uri_selector = selector!("token_uri");
@> let tokenURI_selector = selector!("tokenURI");
​
let mut _calldata: Array<felt252> = array![];
token_id.serialize(ref _calldata);
​
let calldata = _calldata.span();
​
// As we use syscall, the return value is a raw span of serialized data.
// len: 0 -> empty
// len: 1 -> 'old' string
// len > 1 -> ByteArray
@> match starknet::call_contract_syscall( // @audit if first call fails, the whole process will revert
collection_address,
@> token_uri_selector, // The first call is to the token_uri function
calldata,
) {
Result::Ok(span) => span.try_into(),
@> Result::Err(_e) => { // Gracefully handling of errors is not possible in Starknet live chains
match starknet::call_contract_syscall(
collection_address, tokenURI_selector, calldata,
) {
Result::Ok(span) => span.try_into(),
Result::Err(_e) => {
Option::None
}
}
}
}
}

Impact

Impact: High

Likelihood: High

Both token_uri and tokenURI functions are used equally in ERC721 tokens. The issue will arise whenever the latter is used.

Tools Used

Manual Review

Recommendations

Given the current limitations in error handling of Starknet OS, a possible solution to this issue is to explicitly pass as a parameter to token_uri_from_contract_call the implementation used in the particular desired collection:

fn token_uri_from_contract_call(
collection_address: ContractAddress,
token_id: u256,
+ token_uri_getter: ByteArray,
) -> Option<ByteArray> {
// TODO: add the interface detection when the standard is out.
​
- let token_uri_selector = selector!("token_uri");
- let tokenURI_selector = selector!("tokenURI");
​
+ let token_uri_selector = selector!(token_uri_getter);
​
let mut _calldata: Array<felt252> = array![];
token_id.serialize(ref _calldata);
​
let calldata = _calldata.span();
​
// As we use syscall, the return value is a raw span of serialized data.
// len: 0 -> empty
// len: 1 -> 'old' string
// len > 1 -> ByteArray
- match starknet::call_contract_syscall(
- collection_address,
- token_uri_selector,
- calldata,
- ) {
- Result::Ok(span) => span.try_into(),
- Result::Err(_e) => {
- match starknet::call_contract_syscall(
- collection_address, tokenURI_selector, calldata,
- ) {
- Result::Ok(span) => span.try_into(),
- Result::Err(_e) => {
- Option::None
- }
- }
- }
- }
​
// @audit since the correct token URI getter is provided, call_contract_syscall will succeed
+ starknet::call_contract_syscall(collection_address, token_uri_selector, calldata,).try_into();
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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