ArkProject: NFT Bridge

Github
https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L360-L363

Summary

The set_l1_l2_collection_mapping function in the bridge.cairo allows the admin to map an L1 collection address (collection_l1) to an L2 collection address (collection_l2). The function writes these mappings into two mappings: l1_to_l2_addresses and l2_to_l1_addresses. However, if this function is called multiple times with the same collection_l1 or collection_l2 values, the existing mappings will be overwritten without any checks, potentially leading to loss of critical mapping data.

Impact

• Overwriting of Existing Mappings: The current implementation allows existing mappings to be overwritten. This can lead to the accidental loss of the previous association between L1 and L2 addresses, which could disrupt the bridge's operations and lead to incorrect behavior in subsequent interactions with these collections.

• User tokens can get stuck: User tokens can get stuck in escrow forever due to a valid mapping update by the admin, not necessarily a malicious action.

Proof of concept

Let's consider an example scenario to demonstrate the potential issue with the set_l1_l2_collection_mapping function:

1. Initial Deposit: Bob bridges his NFT Everai #1 and NFT Everai #2 from L1 to L2 by calling depositTokens. This action places Everai #1 and Everai #2 into the escrow contract on L1. New tokens are minted for Bob on StarkNet (L2), representing his NFTs.

2. Partial Withdrawal: Bob decides to bridge back NFT Everai #1 to L1. Everai #1 is withdrawn from escrow and returned to L1, but Everai #2 remains in escrow.

3. Admin Mapping Update: The admin calls set_l1_l2_collection_mapping for some reason, updating/overwriting the l1_to_l2_addresses mapping from the original Everai address (ABC) to a new address (XYZ).

4. Unexpected Outcome on Withdrawal: Bob attempts to bridge back Everai #2 from L2 to L1. He expects to receive the original Everai #2 from the original collection with the ABC address. However, due to the update, the l1_to_l2_addresses mapping now points to XYZ. As a result, Bob will receive an NFT from the new collection (XYZ), not the original (ABC).

Recommendation

The implementation should follow the L1-like approach by introducing a force option in the set_l1_l2_collection_mapping function. Overwriting the mappings should be disallowed by default. However, if an overwrite is necessary, the admin should explicitly specify the force option. Before allowing a force update or overwrite, the function should include a check to ensure that no tokens are currently in escrow for the collection address being updated. If tokens are found in escrow, they should be cleared first before proceeding with the force update or overwrite. This approach ensures that mappings are only overwritten when absolutely necessary and that any potential issues related to escrowed tokens are addressed before changes are made.