ArkProject: NFT Bridge

Summary

A malicious actor can stuck other unsuspecting users' tokens forever.

The problem stems from the fact that there's a reentrancy vulnerability in the Escrow::_withdrawFromEscrow() function that can be used on any collection to trap unsuspecting users to lock their tokens permanently if they use the bridge.

Vulnerability Details

The attack can be performed on any collection so let's take Everai for example.

Let's say that I've bridged my NFT with tokenId from L1 -> L2 and vice versa. Currently, the token is sitting in the L1 escrow and I have it on L2.

Now here's how the attack would go.

I bridge my token from L2 -> L1 where I specify the owner_l1 address i.e the to address that will receive the NFT on L1 to be an address to a contract I control with the following functionality:

The code above illustrates the basic functionality of the attacker contract.

Now, I'll call the withdrawFromBridge from my contract which will call the withdrawTokens function on the bridge contract and we'll enter the following functionality:

Let's see what _withdrawFromEscrow does:

Notice how setting the depositor to address(0) with _escrow[collection][id] = address(0x0); is made after the transfer.

Since our token is currently sitting in the escrow on L1, the if is skipped, and the token is transferred with this IERC721(collection).safeTransferFrom(from, to, id);

The important thing to know is that safeTransferFrom will call the onERC721Received function on the receiving address if the address is a contract. Here's the ERC721 implementation:

As you can see, the tokenId is transferred then checkOnERC721Received is called and this is the functionality of the function:

So as confirmed with the code above we established that the token will be transferred and then the onERC721Received function will be called on the receiving contract.

Which, in our case, will deposit the token again to the bridge and bridge it from L1 --> L2. During this _escrow[collection][id] will be set to our contract's address, however, as soon as the onERC721Received is executed i.e the deposit function on the bridge, we'll return to execution on the _withdrawFromEscrow function and we'll execute the last line:

Let's summarize what we did:

• we had tokenId in L1 escrow, and we owned in on L2

• we bridged it from L1 -> L2, withdrew and with reentering we deposited it again from L1 to L2

• execution continues in the _withdrawFromEscrow function and sets _escrow[collection][id] = address(0x0); although this mapping should point to the depositor(us)

Now the token is in the L1 escrow with depositor set as address(0) and we have the token on L2.

If we were to sell this token to any unsuspecting user and that user tries to bridge it, the token will be locked forever.

Let's see why.

He'll bridge it from L2 -> L1, and will call withdrawTokens on L1 which brings us to this functionality again:

When we enter the _withdrawFromEscrow function wasEscrowed will return false:

since the depositor is set to address(0) and this is what the _isEscrowed function is checking:

and withdrawTokens will continue with this:

which will revert since it'll try to mint a tokenId that already exists(it's currently in the bridge contract). Or it'll revert if the the collection was a native L1 collection and mintFromBridge function doesn't exist.

Now the unsuspecting user's tokens are permanently stuck on L1 and L2, and there's nothing he can do to unlock them.

This attack doesn't cost anything other than some bridging fees and can be done for a big % of the total supply of any collection. For example, if that's done on most of the Everai NFTs it's almost guaranteed that some user will try to bridge his NFT and get his tokens stuck.

Impact

User's NFTs are lost forever which greatly damages the reputation of the protocol.

Tools Used

Manual review

Recommendations

One option is to use transferFrom instead of safeTransferFrom. However, an even better option is to just move _escrow[collection][id] = address(0x0); above in the function before any interactions. Like that: