ArkProject: NFT Bridge

Summary

Bridging back tokens to the originator chain will be impossible until the admin steps in and manually adds both collections to the respective mappings, the issue will happen no matter the direction of bridging.

Vulnerability Details

As it was stated in the Discord channel, here issues that can happen when the whitelist is disabled are in scope of the audit.
Now, let’s review what will happen when users try to bridge ERC721 tokens that are not yet configured by the admins:

1. Bridge::depositTokens is called passing any collectionL1 that doesn’t persist in _l1ToL2Addresses and _l2ToL1Addresses mappings, so request::collectionL2 will be address(0)

   Bridge.sol

   function depositTokens(

   uint256 salt,

   address collectionL1,

   snaddress ownerL2,

   uint256[] calldata ids,

   bool useAutoBurn

   )

   external

   payable

   {

   ...MORE CODE

   req.collectionL2 = _l1ToL2Addresses[collectionL1];

   ...MORE CODE

   }

2. Bridging is successful, the transaction is received on Starknet, and bridge::withdraw_auto_from_l1 is called:

   bridge.cairo

   #[l1_handler]

   fn withdraw_auto_from_l1(

   ref self: ContractState,

   from_address: felt252,

   req: Request

   ) {

   ensure_is_enabled(@self);

   assert(self.bridge_l1_address.read().into() == from_address,

   'Invalid L1 msg sender');

   ​

   // TODO: recompute HASH to ensure data are not altered.

   // TODO: Validate all fields the request (cf. FSM).

   ​

   let collection_l2 = ensure_erc721_deployment(ref self, @req);

   ...MORE CODE

3. ensure_erc721_deployment will deploy new ERC721 token, which will be the representation of the Ethereum NFT on L2, and configure the l2_to_l1_addresses and l1_to_l2_addresses, as well as automatically whitelisting it for next bridges that will happen (this is valid only for bridges in the same direction, i.e. only L1 → L2 will work):

   bridge.cairo

   fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {

   ​

   let l1_req: EthAddress = *req.collection_l1;

   let l2_req: ContractAddress = *req.collection_l2;

   ​

   let collection_l2 = verify_collection_address(

   l1_req,

   l2_req,

   self.l2_to_l1_addresses.read(l2_req),

   self.l1_to_l2_addresses.read(l1_req),

   );

   ​

   if !collection_l2.is_zero() {

   return collection_l2;

   }

   ​

   let hash = *req.hash;

   let salt_data: Span<felt252> = array![hash.low.into(), hash.high.into()].span();

   let salt = poseidon_hash_span(salt_data);

   ​

   let l2_addr_from_deploy = deploy_erc721_bridgeable(

   self.erc721_bridgeable_class.read(),

   salt,

   req.name.clone(),

   req.symbol.clone(),

   req.base_uri.clone(),

   starknet::get_contract_address(),

   );

   ​

   self.l1_to_l2_addresses.write(l1_req, l2_addr_from_deploy);

   self.l2_to_l1_addresses.write(l2_addr_from_deploy, l1_req);

   ... MORE CODE

   }

4. User decides he doesn’t need to keep the token in L2 anymore, or he is forced to bridge it back to L1, for example his rent period is expiring and he can lose his collateral if he doesn’t return it in time. bridge::deposit_tokens is called which will set collectionL1 address to the real NFT address on L1 and send the tx.

5. Payload is bridged successfully and the user is in a hurry to redeem his NFT so he can return it and calls Bridge::withdrawTokens with the valid payload, that has collectionL1 = real ERC721 address, but when CollectionManager::_verifyRequestAddresses is called the following revert will be caused:

CollectionManager.sol

InvalidCollectionL1Address will be thrown until the admin doesn’t step in and call the Bridge::setL1L2AddressMapping. But it will be late and the user will lose his collateral that he provided to the rental services. Here we can’t talk about user mistakes since non-technical users, and even technical are not obligated to inspect carefully the contracts and then retrieve their tokens, also when they already bridged their tokens to L2 successfully will means that there are no issues that can arise when tokens are retrieved back on L1.

Impact

Loss of funds and blockage of L1 → L2 → L1 bridge due to missing mapping configuration.

Tools Used

Manual Review

Recommendations

Consider setting the L1-to-L2 collections in deposit functions so when tokens are bridged back the original collection address is used.