Summary

NFT can get stuck in escrow on L2 side if incorrect collection_l1 address was set in the l2_to_l1_addresses mapping on the L2 side of the bridge. This will cause a revert on the L1 side when consuming the message through the withdrawTokens function in Bridge.sol

Vulnerability Details

The admin could set an incorrect mapping through set_l1_l2_collection_mapping function.
The deposit_tokens function gets the collection_l1 address from the mapping here https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L274

The transaction is then sent to L1.

The withdrawTokens function on L1 side calls the _verifyRequestAddresses function here https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L179C32-L179C55

This function will revert here https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/CollectionManager.sol#L138

Since collection_l1 address was set in the request from L2 and didnt match with what is present in the mapping on L1 side, the transaction will revert. The admin could change the address in the mapping on L1 side but the withdraw transaction would be successful only if this address is set to what was sent in the request (which might not even be the address of a contract on chain).

Impact

The impact could severe and result in loss of NFT for the user which will remain stuck in escrow on the L2 side.

Tools Used

Manual review and snforge test

Recommendations

The mapping on L2 side should be set through Starknet Messaging call from the L1 side rather than being set independently. This will ensure there is only 1 source of truth for such sensitive data and ensure L1 and L2 sides remain in sync. Of course, a transaction could go through from L2 side after the request for mapping change is sent from L1 side but before it can be consumed on L2 side. But this could be mitigated by unwhitelisting the collection on L2 side before making any changes to the mapping.