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

Token URIs are not propagated to the minted NFTs on L1 when using `ERC721Bridgeable`.

said

Summary

When users bridge a token from L2 to L1 and the ERC721 on L1 is not escrowed (meaning it uses ERC721Bridgeable), the L2 token's URI will not be propagated or stored in the newly minted NFTs on L1, potentially causing the NFT to lose its value.

Vulnerability Details

Users can bridge their L2 NFTs to L1 by calling deposit_tokens. During this operation, the bridged NFT's data, including the token's base_uri and uris (if they exist), will be populated on Request data, then the data will be send to L1.

fn deposit_tokens(
ref self: ContractState,
salt: felt252,
collection_l2: ContractAddress,
owner_l1: EthAddress,
token_ids: Span<u256>,
use_withdraw_auto: bool,
use_deposit_burn_auto: bool,
) {
// ...
escrow_deposit_tokens(ref self, collection_l2, from, token_ids);
​
let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
​
let req = Request {
header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
collection_l1,
collection_l2,
owner_l1,
owner_l2: from,
name,
symbol,
>>> base_uri,
ids: token_ids,
values: array![].span(),
>>> uris,
new_owners: array![].span(),
};
​
let mut buf: Array<felt252> = array![];
req.serialize(ref buf);
​
starknet::send_message_to_l1_syscall(
self.bridge_l1_address.read().into(),
buf.span(),
)
.unwrap_syscall();
self.emit(DepositRequestInitiated {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}

However, when user's call withdrawTokens on L1 to retrieve the NFTs, if the collection is not escrowed, it will mint the collection to the users, providing the corresponding token ids, but not set or store token's base uri or uris.

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L203-L209

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
// ...
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
>>> IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
​
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
​
return collectionL1;
}

Impact

This could potentially cause the bridged NFTs to lose value, as they may not properly display the collection's base URI or token URIs, which are often the main value proposition for most NFTs.

Tools Used

Manual review

Recommendations

Provide or set the URI or base URI when minting the NFTs on L1

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.

Support

FAQs

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