ArkProject: NFT Bridge

Summary

The Cairo contract's set_l1_l2_collection_mapping function lacks validation checks found in the equivalent Solidity function, leading to potential risks of unintended overwriting of existing collection mappings. This inconsistency can result in errors and unintended changes to mappings.

Vulnerability Details

In the Cairo contract, the set_l1_l2_collection_mapping function allows for the direct mapping of L1 and L2 collection addresses without performing any checks to verify if a mapping already exists. This approach assumes the operation is always valid. Conversely, the Solidity contract's _setL1L2AddressMapping function includes a critical check that prevents overwriting an existing mapping unless a force parameter is set to true. If the mapping already exists and the force parameter is not enabled, the function reverts with a CollectionMappingAlreadySet error, thereby protecting against accidental or unintended changes.

Locations:

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

• https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L360C9-L364C10

Impact

Without validation checks in the Cairo contract, there is a risk of unintentionally overwriting existing mappings, which could lead to errors and disrupt the integrity of the L1-L2 collection mappings. This inconsistency creates a medium impact, as incorrect mappings could affect system functionality and user trust.

Tools Used

• Manual code review

Recommendations

• Introduce validation checks in Cairo Contract to verify whether a mapping already exists before writing a new one. This check should prevent overwriting existing mappings unless explicitly intended, mirroring the logic in the Solidity contract. If a mapping already exists, the function should revert with an appropriate error message.

• Add optional force parameter similar to the Solidity implementation. This parameter would allow overwriting an existing mapping only when necessary, providing flexibility while maintaining safeguards against accidental changes.