ArkProject: NFT Bridge

Summary

The safeTransferFrom functions in OpenZeppelin's ERC721 and ERC1155 trigger external calls to user addresses, which may expose the contract to reentrancy attacks. The current implementation of _withdrawFromEscrow does not include a nonReentrant modifier and fails to adhere to the check-effects-interactions pattern, as _escrow[collection][id] is cleared only after the external token transfer is executed.

Vulnerability Details

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

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L79
the withdrawTokens() do a call to _withdrawFromEscrow() to transfer tokens

In _withdrawFromEscrow() the _escrow[collection][id] variable is cleared after the external token transfer via safeTransferFrom. This breaks the check-effects-interactions pattern and leaves the contract vulnerable to reentrancy attacks during the external call.

Attack Path

User Escrows Token: The user escrows an ERC721 or ERC1155 token in the contract.

Malicious Contract as Receiver: The attacker creates a malicious contract that implements onERC721Received or onERC1155Received, containing reentrant code that calls back into the vulnerable contract.

Token Withdrawal Initiated: The attacker calls the withdrawTokens function to withdraw the token from the escrow.

Reentrant Call Triggered: During the call to safeTransferFrom, the malicious contract's onERC721Received or onERC1155Received function is triggered, allowing the reentrant call to execute before _escrow[collection][id] is cleared.

Exploit Reentrancy: The malicious contract repeatedly calls withdrawTokens while the original call is still executing, potentially withdrawing multiple tokens or manipulating the contract's state.

Impact

This reentrancy can allow the attacker to withdraw multiple tokens in a single transaction. By specifying different token IDs in each reentrant call, the attacker can continue draining the escrow until the contract updates its internal state to mark the tokens as withdrawn.

Tools Used

Manual review

Recommendations

I recommend both of these solutions though either one will be sufficient on its own:

Add nonReentrant modifier
Set _escrow[collection][id] = address(0x0) before performing IERC1155/IERC721(collection).safeTransferFrom() to apply the checks-effects-interactions pattern.