ArkProject: NFT Bridge

Summary

1. Functions updating the class hash do not check new class hash appropriately in Cairo contracts

2. camelCase function call not supported in escrow_deposit_tokens when transferring tokens in bridge.cairo

3. Inside _white_list_collection function in bridge.cairo file, check for whitelisted status (active) is not necessary

4. _whitelistCollection function in Bridge.sol should check whether collection implements IERC721 before whitelisting

5. depositTokens function in Bridge.sol does not check that owner_l2 is non-zero before sending message to L2 contract - this will cause transaction revert on L2 and message wont get consumed on Starknet

Vulnerability Details

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

   In the above function, replace_class_syscall is called without checking whether new class still has the upgrade functionality. It is good practice to check that using ISRC5 (equivalent to IERC165) which confirms that new contract has upgrade function.
   https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.11.0/src/introspection/src5.cairo

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

   In the above function, the class hash is being written to the storage without verifying that it has been declared (calling replace_class_syscall ensures that the ClassHash is atleast declared) and supports IERC721. To check that it has been declared & supports IERC721, it is enough to make a library_call_syscall using the new class hash and use function supports_interface from ISRC5

3. https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L369
   Again in the above function, class hash is being upgraded without checking whether appropriate IERC721 is implemented by the new class.

4. https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L418
   escrow_deposit_tokens function does not support tokens with camelCase and only calls the snake_case function transfer_from while transferring the tokens. If any token with camelCase functions only is whitelisted then it will revert the transaction.

5. https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L491
   Inside _white_list_collection function in bridge.cairo file, check for whitelisted (active variable in code) status is not necessary ref line https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L509 - This is because a collection is whitelisted only when enabled=true, and it is removed from the list when enabled=false. Thus, there is never a situation where a collection will be in the whitelist in enabled=false state or !active state. Hence, this check can be removed to make code simpler and save on gas fees.

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

   _whitelistCollection function in Bridge.sol does not check a collection for implementation of IERC721 interface before whitelisting.

7. https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L88
   depositTokens function in Bridge.sol does not check that owner_l2 is non-zero (it only checks for valid felt) before sending message to L2 contract - The OZ implementation of ERC721 being used on starknet side reverts when "to" address is 0 ref https://github.com/OpenZeppelin/cairo-contracts/blob/a83f36b23f1af6e160288962be4a2701c3ecbcda/src/token/erc721/erc721.cairo#L446 . The user will not lose the NFT on the L1 side since message wont get consumed on starknet but will have to cancel the transaction on Ethereum side to get back the NFT after waiting period between startRequestCancellation and cancelRequest call.

Impact

The probability of admin changing to an incorrect class hash is generally quite low but should it happen the impact could be severe.

Changing to an incorrect ERC721 class hash also has low impact because the admin can change it back but if there is any transaction from L1 side in the meantime then the message wont get consumed on starknet and user will have to cancel the transaction on Ethereum side to recover the NFT stuck in escrow on L1.

Not supporting camelCase is also low impact simply because the admin can choose not to whitelist such a collection with camelCase only functions.

Again, not checking IERC721 compatibility on L1 side before whitelisting is not a deal breaker since the admin will probably do due diligence before whitelisting any contract. It becomes an issue when the bridge is truly permissionless with anyone able to whitelist any contract. It also becomes an issue if a transaction from L2 side comes through before the admin can rectify the mistake since then the NFT would get stuck in Escrow on L2 side (depositTokens function also checks the collection contract for support of IERC721 interface before sending message to L2)

Last issue is a bit tricky, the user is ultimately responsible for setting an incorrect l2 address but checking for non-zero owner_l2 is a relatively simple check and spares the extra work and gas fees to cancel the transaction and recover NFT from escrow on L1.

Tools Used

Manual review

Recommendations

1. Checking for support of an upgrade interface can be done like

   let is_interface_supported:bool = ISRC5LibraryDispatcher{class_hash: new_class_hash}.supports_interface(<interface_id_for_upgrade>);This will also serve to check whether class-hash is declared (which is required before writing new ERC721 class hash in storage).

2. Since try-catch is not yet supported on Starknet, we need a way to store whether ERC721 contract supports snake_case or camelCase when whitelisting.

and while transferring tokens we can do the following