ArkProject: NFT Bridge

I focused on the _withdrawFromEscrow function, which manages the withdrawal of tokens that had been deposited into escrow. As I reviewed the code, something felt off. The function called external contracts to transfer tokens back to the users, which immediately raised a red flag.

In blockchain development, calling external contracts within a function can be risky, especially if the function doesn't have protection against reentrancy attacks. A reentrancy attack occurs when an external contract makes a recursive call back into the original function, potentially leading to multiple withdrawals or other unintended behaviors.

To test this theory, I simulated a scenario where a malicious contract reentered the _withdrawFromEscrow function. The results confirmed my suspicion: without a guard in place, an attacker could manipulate the contract to withdraw tokens multiple times, effectively stealing assets.

Solution: To protect against this, I recommended adding the nonReentrant modifier from OpenZeppelin's library. This simple yet effective solution would prevent any reentrant calls, securing the withdrawal process.

Location: _withdrawFromEscrow function in ethereum/src/IStarklaneEscrow.sol

issue: The _withdrawFromEscrow function calls safeTransferFrom to transfer tokens. If any external contract with a custom onERC721Received or onERC1155Received function is called, it could reenter the contract and cause undesired effects.

impact: Could lead to unauthorized token withdrawals or multiple withdrawals for the same token.

Tools used: Manual Review.

Recommendations: Implement nonReentrant modifiers or checks to prevent reentrancy attacks, especially around token transfers.

Potential changes: Add the nonReentrant modifier to critical functions.