Github

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L368-L375

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/CollectionManager.sol#L111-L134

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L153-L215

Summary

User tokens can get stuck in escrow forever if setL1L2CollectionMapping is called and it updates _l2ToL1Addresses.

Vulnerability Details

setL1L2CollectionMapping is an admin-controlled function that updates _l1ToL2Addresses and _l2ToL1Addresses mappings in CollectionManager. The problem with this function is that it can overwrite already saved collections due to the force parameter in _setL1L2AddressMapping. This overwrite can lead to a critical issue where user tokens get stuck in escrow forever.

When the _l1ToL2Addresses and _l2ToL1Addresses mappings are updated, it directly impacts the withdrawTokens function, which uses _verifyRequestAddresses for request verification. When an already used collection address is updated in _l2ToL1Addresses, the function _verifyRequestAddresses will return the new updated collection address, not the old/original one. If the new collection address is returned, the withdrawTokens function will mint a token from the new updated collection to the user instead of returning the original escrowed token.

This results in the user's original token being stuck in escrow forever while they receive a new token from the updated collection.

Impact

Although the admin is a trusted role, making the likelihood of this issue low, the impact is very high. User tokens can get stuck in escrow forever due to a valid update by the admin, not necessarily a malicious action.

Proof of Concept

Let's take an example scenario to understand the bug in detail:

1. 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. 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. The admin calls setL1L2CollectionMapping for some reason, updating/overwriting _l2ToL1Addresses from the original Everai address (ABC) to a new address (XYZ).

4. 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 _l2ToL1Addresses mapping now points to XYZ. As a result, Bob will receive an NFT from the new collection (XYZ), not the original (ABC).

Recommendation

Overwriting the mappings should not be allowed in the first place. However, if it is necessary, there should be a check in setL1L2CollectionMapping to ensure that no tokens are in escrow for the collection address being updated. If there are tokens, they should be cleared first, and then the force update/overwrite should be done.