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

Issue with bridging back an L1-native NFT from L2 to L1

fyamf

Summary

When the first token of a collection is going to be bridged from L1 to L2, the address of the L2 collection is not known on L1 yet. So, req.collectionL2 = 0. On L2 side, during withdrawal, L2 collection will be deployed and the mappings l1_to_l2_addresses and l2_to_l1_addresses would be updated accordingly. So far so good.

When this NFT of the L2 collection is going to be bridged from L2 to L1, an issue happens that reverts the transaction. Because, during withdrawal on L1 side, the mapping _l2ToL1Addresses[collectionL2Req] returns address(0) (because the L1 collection associated with the L2 collection is not known on L1). As a result, since l1Mapping != l1Req, it reverts by InvalidCollectionL1Address.

Vulnerability Details

  • Suppose the first token of a collection is going to be bridged from L1 to L2.

  • During depositing the NFT on L1, since the associated L2 collection is not yet deployed on L2, req.collectionL2 would be 0.

req.collectionL2 = _l1ToL2Addresses[collectionL1];

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

  • On L2 side, when the function withdraw_auto_from_l1 is invoked, it ensures the L2 collection deployment by calling the internal function ensure_erc721_deployment.

let collection_l2 = ensure_erc721_deployment(ref self, @req);

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L141

  • In this function, since L2 collection associated with L1 collection is not available, i.e. l2_req = 0 and l2_bridge = 0, collection_L2 would be zero. As a result, it will be deployed.

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),
);

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L433

fn verify_collection_address(
l1_req: EthAddress,
l2_req: ContractAddress,
l1_bridge: EthAddress,
l2_bridge: ContractAddress,
) -> ContractAddress {
​
// L1 address must always be set as we receive the request from L1.
if l1_req.is_zero() {
panic!("L1 address cannot be 0");
}
​
// L1 address is present in the request and L2 address is not.
if l2_req.is_zero() {
if l2_bridge.is_zero() {
// It's the first token of the collection to be bridged.
return ContractAddressZeroable::zero();
}
} else {
// L1 address is present, and L2 address too.
if l2_bridge != l2_req {
panic!("Invalid collection L2 address");
}
​
if l1_bridge != l1_req {
panic!("Invalid collection L1 address");
}
}
​
l2_bridge
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/collection_manager.cairo#L170

  • After deployment, the mappings l1_to_l2_addresses and l2_to_l1_addresses would be updated:

self.l1_to_l2_addresses.write(l1_req, l2_addr_from_deploy);
self.l2_to_l1_addresses.write(l2_addr_from_deploy, l1_req);

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L457-L458

  • So far everything is working properly.

  • Now suppose, this collection on L2 is going to be bridged to L1.

  • When the function deposit_tokens is called, the mapping l2_to_l1_addresses is used to get the associated collection on L1. Then, the fields collection_l1 and collection_l2 in the struct Request would be populated with nonzero values.

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(),
};

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L274-L290

  • On L1 side, when calling the function withdrawTokens, it calls the internal function _verifyRequestAddresses to calculate the address of the associated collection on L1.

address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);

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

  • In the function _verifyRequestAddresses, since l1Req > address(0) and l2Req > 0 (both of these values are forwarded from L2), the body of first if-clause will not be executed.

// L2 address is present in the request and L1 address is not.
if (l2Req > 0 && l1Req == address(0)) {
if (l1Mapping == address(0)) {
// It's the first token of the collection to be bridged.
return address(0);
} else {
// It's not the first token of the collection to be bridged,
// and the collection tokens were only bridged L2->L1.
return l1Mapping;
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/CollectionManager.sol#L125

  • Then, the body of second if-clause will be executed.

// L2 address is present, and L1 address too.
if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {
revert InvalidCollectionL1Address();
} else if (l2Mapping != l2Req) {
revert InvalidCollectionL2Address();
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/CollectionManager.sol#L137

  • The issue happens here. Since address l1Mapping = _l2ToL1Addresses[collectionL2Req];, the value of l1Mapping is address(0), because the mapping _l2ToL1Addresses is not aware of the L1 collection associated with L2 collection. So, the body of the first inner if-clause will be executed, and reverts the transaction.

if (l1Mapping != l1Req) {
revert InvalidCollectionL1Address();
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/CollectionManager.sol#L138

Please note that:
This finding is almost similar to the report with title "Issue with bridging back an L2-native NFT from L1 to L2", but the root cause and fix are different. In other words, this finding explains the issue when a collection is native to L1, then bridged to L2, and later bridged back to L1. But, the other report explains an issue when a collection is native to L2, then bridged to L1, and later bridged back to L2.

Impact

  • Stuck of NFT. Because the NFT is escrowed on L2, but it is not released on L1 due to the revert during withdrawal.

Tools Used

Recommendations

It should be modified as follows:

// L2 address is present, and L1 address too.
if (l2Req > 0 && l1Req > address(0)) {
- if (l1Mapping != l1Req) {
+ if (l1Mapping != l1Req && l1Mapping != address(0)) {
revert InvalidCollectionL1Address();
- } else if (l2Mapping != l2Req) {
+ } else if (l2Mapping != l2Req && l2Mapping != 0) {
revert InvalidCollectionL2Address();
+ } else if (l1Mapping == address(0) || l2Mapping == 0) {
+ _l2ToL1Addresses[collectionL2Req] = l1Req;
+ _l1ToL2Addresses[l1Req] = collectionL2Req;
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/CollectionManager.sol#L137

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-first-bridgeof-a-collection-L1<->L2-do-not-sync-addresses

Likelyhood: High, any collections bridged, without bridge owner action, will be unable to bridge back. Impact: High, L2 -> L1 tokens will be stuck in the bridge. L1 -> L2 will need to ask for a cancellation.

Support

FAQs

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