Summary
When bridging an ERC721 collection that doesn't support IERC721Metadata from Ethereum to Starknet, the transaction succeeds on the Ethereum side and the NFTs are deposited into the escrow contract on the L1.
If this is the first NFT from its collection and its’ L2 address has not been whitelisted, the bridge contract deploys an 'empty' collection on Starknet with no name, symbol, or URI pertaining to the collection.
This occurs because the bridge contract doesn't properly handle collections lacking metadata support or validate the required fields from the request struct on the starknet side, potentially confusing users and diminishing the functionality of bridged NFTs.
Vulnerability Details
In Bridge.sol, the collection metadata is retrieved via the function erc721Metadata(collectionL1, ids)
if (ctype == CollectionType.ERC721) {
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil
.erc721Metadata(collectionL1, ids);
(https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L121)
In TokenUtil.sol, if the collection does not support the IERC721Metadata interface, empty strings will be returned for the name, symbol, URI and an empty array will be returned for the tokenURI fields.
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));
}
}
https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/TokenUtil.sol#L84
In Bridge.cairo, for a new collection being bridged to the L2, if the collection address is not present in the mappings, a new contract will be deployed using deploy_erc721_bridgeable(..)
#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
ensure_is_enabled(@self);
assert(self.bridge_l1_address.read().into() == from_address,
'Invalid L1 msg sender');
let collection_l2 = ensure_erc721_deployment(ref self, @req);
// rest of the code
if is_escrowed {
// rest of the code
} else {
if (req.uris.len() != 0) {
// rest of the code
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}
i += 1;
}
}
fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
let l1_req: EthAddress = *req.collection_l1;
let l2_req: ContractAddress = *req.collection_l2;
let collection_l2 = verify_collection_address(
l1_req,
l2_req,
self.l2_to_l1_addresses.read(l2_req),
self.l1_to_l2_addresses.read(l1_req),
)
if !collection_l2.is_zero() {
return collection_l2;
}
let hash = *req.hash;
let salt_data: Span<felt252> = array![hash.low.into(), hash.high.into()].span();
let salt = poseidon_hash_span(salt_data)
let l2_addr_from_deploy = deploy_erc721_bridgeable(
self.erc721_bridgeable_class.read(),
salt,
req.name.clone(),
req.symbol.clone(),
req.base_uri.clone(),
starknet::get_contract_address(),
)
// rest of the code
}
In deployerc721bridgeable(), the empty bytearrays are directly serialzied into the calldata as there is no check whether the name, symbol and base_uri is empty.
fn deploy_erc721_bridgeable(
class_hash: ClassHash,
salt: felt252,
name: ByteArray,
symbol: ByteArray,
base_uri: ByteArray,
bridge_address: ContractAddress,
) -> ContractAddress {
let mut calldata: Array<felt252> = array![];
name.serialize(ref calldata);
symbol.serialize(ref calldata);
base_uri.serialize(ref calldata);
calldata.append(bridge_address.into());
// For now, the bridge is by default the collection owner.
calldata.append(bridge_address.into());
// Last argument false -> set the address of the contract using this function
match starknet::deploy_syscall(class_hash, salt, calldata.span(), false) {
Result::Ok((addr, _)) => addr,
// TODO: do we want an event emitted here?
Result::Err(revert_reason) => panic(revert_reason)
}
}
Summary
Similarly, when an NFT is bein bridged from Starknet to Ethereum, there is a chance that an empty NFT collection is deployed by the bridge contract in the L1.
Vulnerabilities
In Bridge.cairo, if the collection metadata was not retrieved successfully, empty strings are set.
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,
) {
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids))
let (name, symbol, base_uri, uris) = match erc721_metadata {
Option::Some(data) => (data.name, data.symbol, data.base_uri, data.uris),
Option::None => ("", "", "", array![].span())
}
}
In Bridge.sol, these empty strings are used to deploy a new collection.
function withdrawTokens(
uint256[] calldata request
) external payable returns (address) {
if (!_enabled) {
revert BridgeNotEnabledError();
}
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
}
Impact
This results in the deployment of ERC721 contracts on starknet/ethereum with empty name, symbol and URI. The deployed contract has no identifying information, and users might believe there is something wrong with the bridging when they view "blank/empty" nfts. This affects the core bridging functionality of the protocol.
Tools Used
Manual review
Recommendations
To address this,
fn deploy_erc721_bridgeable(
) -> ContractAddress {
assert(!name.is_empty(), "Name cannot be empty");
assert(!symbol.is_empty(), "Symbol cannot be empty");
assert(!base\_uri.is_empty(), "Base URI cannot be empty")
}
fn deploy_erc721_bridgeable(
) -> ContractAddress {
let name = if name.is_empty() {
ByteArray::from_string("Bridged Collection")
} else {
name
};
let symbol = if symbol.is_empty() {
ByteArray::from_string("ARKBRIDGE")
} else {
symbol
};
let base_uri = if base_uri.is_empty() {
ByteArray::from_string("https://default-uri.com/")
} else {
base_uri
};
