ArkProject: NFT Bridge

Summary

The _withdrawFromEscrow function in a bridge contract is responsible for transferring ERC721 NFTs back to users by using the safeTransferFrom method. This method triggers a callback function for Smart Contract wallets. Due to a vulnerability in this process, an attacker can exploit this callback to make an NFT permanently non-withdrawable.

Vulnerability Details

To understand this issue, let’s walk through a typical scenario:

1. Initial Setup:

   • An attacker owns an NFT and initiates a bridge action by calling the depositTokens function.

   • This action sets an internal flag called _escrow to a non-zero value, indicating that the NFT is now held in the bridge contract.

2. Attempted Withdrawal:

   • The attacker then attempts to withdraw the NFT from Starknet or decides to cancel the bridging action.

   • This triggers the _withdrawFromEscrow function, which is called either through the withdrawTokens function or the cancelRequest function.

Here is the relevant code snippet:

3. Re-entrancy Exploit:

   • In the code above, the safeTransferFrom function is used to transfer the NFT back to the owner. For Smart Contract wallets, this function triggers the onERC721Received callback.

   • During this callback, the attacker can re-initiate the depositTokens function to bridge the NFT back to Starknet. This action sets the _escrow flag to a non-zero value again.

   • Once the callback completes, the _escrow flag is reset, incorrectly indicating that the NFT is no longer held by the bridge contract, even though it actually is.

4. Consequences:

   • If someone later buys this NFT on a Starknet marketplace, they will be unable to bridge the NFT back to L1 because the withdrawTokens function will fail, making the NFT effectively non-withdrawable.

Impact

An attacker can exploit this vulnerability to make NFTs permanently unbridgeable, preventing users from withdrawing them.

Tools Used

Manual Review

Recommendations

1. Apply the Checks-Effects-Interactions (CEI) Pattern:

   • To prevent this vulnerability, modify the _withdrawFromEscrow function to immediately reset the _escrow flag before any external interactions (like transferring the NFT). This prevents the attacker from exploiting the callback to alter the contract’s state.

   Here’s how the code could be modified:

   function _withdrawFromEscrow(

   CollectionType collectionType,

   address collection,

   address to,

   uint256 id

   )

   internal

   returns (bool)

   {

   if (!_isEscrowed(collection, id)) {

   return false;

   }

   ​

   + _escrow[collection][id] = address(0x0);

   ​

   address from = address(this);

   ​

   if (collectionType == CollectionType.ERC721) {

   IERC721(collection).safeTransferFrom(from, to, id);

   } else {

   // TODO:

   // Check here if the token supply is currently 0.

   IERC1155(collection).safeTransferFrom(from, to, id, 1, "");

   }

   ​

   - _escrow[collection][id] = address(0x0);

   ​

   return true;

   }

2. Implement nonReentrant Modifier:

   • To further safeguard against re-entrancy attacks, add a nonReentrant modifier to the depositTokens and withdrawTokens functions. This will prevent the same function from being called multiple times in a single transaction, thereby blocking re-entrancy attacks.