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

Collections with a `baseURI` cannot properly be bridged to L2 because L2 only supports an array of URIs

n4nika

Summary

If a user tries to bridge an NFT of a L1-native collection that uses a baseURI to L2, the NFT's metadata will not be reflected on L2 causing loss of information about the NFT.

Vulnerability Details

The L2 bridge, when withdrawing NFTs with withdraw_auto_from_l1, only mints NFTs with uris if there are uris in the uris array of the request. L1 on the other hand uses a baseURI if the collection supports it. This means that if we bridge an NFT with a baseURI to L2, the NFT's uri will not be reflected as it will be minted with mint_from_bridge instead of mint_from_bridge_uri.

fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
// [...]
​
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
​
let token_id = *req.ids[i];
​
let to = req.owner_l2;
let from = starknet::get_contract_address();
​
let is_escrowed = !self.escrow.read((collection_l2, token_id)).is_zero();
​
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
} else {
if (req.uris.len() != 0) { // [1] <-----
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}
​
i += 1;
};
​
self.emit(WithdrawRequestCompleted {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}

This can be seen at [1]. If uris is empty we do not mint with a URI even if there is a base_uri specified in the request.

Impact

This means that a NFT's uri may not be reflected on L2 when bridging. Since the uri points to crucial information about an NFT like the image associated with it, this causes major inconvenience for the users, impacting the protocol's reputation.

Proof of Concept

Since the baseURI functionality on L1 is currently broken, building an end-to-end PoC for this requires fixing this issue first. Therefore I will step through the code to show the bug.

Initial state:

  • NFT deployed on L1 with baseURI

  • NFT deployed on L2 without any metadata yet as it is just there to receive the L1-native NFTs with their metadata

Entrypoint: Bridge::depositTokens

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// [...]
​
req.ownerL1 = msg.sender;
req.ownerL2 = ownerL2;
​
if (ctype == CollectionType.ERC721) {
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil.erc721Metadata( // [1] <-----
collectionL1,
ids
);
} else {
(req.uri) = TokenUtil.erc1155Metadata(collectionL1);
}
​
_depositIntoEscrow(ctype, collectionL1, ids);
​
req.tokenIds = ids;
​
uint256[] memory payload = Protocol.requestSerialize(req);
if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

Here at [1], we retrieve the token's metadata. If the token has a baseURI set, this should be taken, leaving the tokenURIs empty.

TokenUtil::erc721Metadata

function erc721Metadata(
address collection,
uint256[] memory tokenIds
)
internal
view
returns (string memory, string memory, string memory, string[] memory)
{
// [...]
​
(bool success, string memory _baseUri) = _callBaseUri(collection);
if (success) {
return (c.name(), c.symbol(), _baseUri, new string[](0)); // [2] <-----
}
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);
}
}

This can be seen at [2].

Now let's look at the receiving side (L2):

bridge::withdraw_auto_from_l1

fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
// [...]#
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
​
let token_id = *req.ids[i];
​
let to = req.owner_l2;
let from = starknet::get_contract_address();
​
let is_escrowed = !self.escrow.read((collection_l2, token_id)).is_zero();
​
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
} else {
if (req.uris.len() != 0) { // [3] <-----
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());
} else { // [4] <-----
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}
​
i += 1;
};
​
self.emit(WithdrawRequestCompleted {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}

At [3], we would handle the minting with URIs if the uris array is non-empty. Since our request only contains a base_uri and no uris, this path is not taken. Instead we take the path at [4], meaning we call mint_from_bridge which does not touch the NFT's metadata.

erc721_bridgeable::mint_from_bridge

fn mint_from_bridge(ref self: ContractState, to: ContractAddress, token_id: u256) {
// [...]
self.erc721._mint(to, token_id);
}

Tools Used

Manual review

Recommended Mitigation

This can be fixed by adding another check after if (req.uris.len() != 0) for whether req contains a base_uri and if so, mint the NFT with the proper URI, ensuring its metadata is preserved on L2.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months 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.