ArkProject: NFT Bridge

Summary

The functions setL1L2CollectionMapping in Solidity and set_l1_l2_collection_mapping in Cairo lack checks to ensure that no NFTs are escrowed for the collection before altering the collection mapping. This oversight can lead to potential loss of NFTs if the owner changes the collection mapping while NFTs are still held in escrow.

Vulnerability Details

The function setL1L2CollectionMapping in solidity and set_l1_l2_collection_mapping in cairo doesnt do any checks on whether the contract has any NFTs escrowed for the collection. This can lead to users loosing their NFTs if the owner changes the collection mapping.

Locations:

• 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/starknet/src/bridge.cairo#L360-L364

Impact

Although the likelihood of an issue arising from this vulnerability may be low (since it depends on the presence of escrowed NFTs and an owner’s decision to update the mapping), the impact is high. Users could lose their NFTs if the collection mapping is updated while NFTs are still escrowed. This can lead to substantial asset loss and damage to user trust.

Tools Used

• Manual code review

Recommendations

• Add an additional check that puts the users NFTs in safety before changing the collection mapping, this can be a simple counter of escrowed NFTs for the collection.