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

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

fyamf

Summary

If the first token of a collection on L2 is going to be bridged from L2 to L1, since the associated L1 collection address is not known yet, the forwarded address would be address(0). On L1 side, during withdrawal, the L1 collection would be updated and the mappings _l1ToL2Addresses and _l2ToL1Addresses would be updated accordingly.

The issue is when this NFT on L1 is going to be bridged back to L2. On L2 side, during withdrawal, the mapping l1_to_l2_addresses returns 0 as it does not know the L2 collection associated with the L1 collection. So, during verification, it reverts by Invalid collection L2 address. Since the withdrawal on L2 is auto, retrying such withdrawal is not possible, so it would be impossible to withdraw the NFT on L2, and the NFT would be stuck on L1 permanently.

Vulnerability Details

  • When the first token of an L2 collection is bridged from L2 to L1, during the function call deposit_tokens, the field collection_l1 would be address(0) because the L1 collection is not yet deployed and it is not known yet.

let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);

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

  • On L1 side, during withdrawal, the L1 collection will be deployed as collectionL1 = address(0).

address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
​
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
​
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
// update whitelist if needed
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}

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

// 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#L128

  • After deployment the mappings _l1ToL2Addresses and _l2ToL1Addresses would be updated.

_l1ToL2Addresses[proxy] = collectionL2;
_l2ToL1Addresses[collectionL2] = proxy;

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

The issue happens hereafter:

  • If this NFT on L1 is going to be bridged back to L2, during the deposit, both req.collectionL1 and req.collectionL2 are known and are forwarded as parameters to L2.

req.collectionL1 = collectionL1;
req.collectionL2 = _l1ToL2Addresses[collectionL1];

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

  • On L2 side, during withdrawal, it invokes the internal function ensure_erc721_deployment to calculate the L2 collection associated with this forwarded L1 collection.

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, both req.collection_l1 and req.collection_l2 are known (since they are forwarded as payload from L1). But, the mappings self.l2_to_l1_addresses.read(l2_req) and self.l1_to_l2_addresses.read(l1_req) return address(0) and 0, respectively. Because, L2 is not aware of recently-deployed L1 collection.

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

  • As a result, in the function verify_collection_address, since l2_bridge = 0 and l2_req > 0 we have l2_bridge != l2_req, so it will revert by Invalid collection L2 address.

// L1 address is present, and L2 address too.
if l2_bridge != l2_req {
panic!("Invalid collection L2 address");
}

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

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

Impact

  • Stuck of an NFT on L1 without being withdrawable on L1 and L2.

Tools Used

Recommendations

The following modifications are recommended:

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 {
+ if l2_bridge != l2_req && l2_bridge != starknet::contract_address_const::<0>() {
panic!("Invalid collection L2 address");
}
​
- if l1_bridge != l1_req {
+ if l1_bridge != l1_req && l1_bridge != starknet::contract_address_const::<0>() {
panic!("Invalid collection L1 address");
}
+ if l1_bridge == starknet::contract_address_const::<0>() || l2_bridge == starknet::contract_address_const::<0>() {
+ self.l1_to_l2_addresses.write(l1_req, l2_req);
+ self.l2_to_l1_addresses.write(l2_req, l1_req);
+ l2_bridge = l2_req;
+ }
}
​
l2_bridge
}

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

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.