ArkProject: NFT Bridge

Summary:

In the _depositIntoEscrow function, the contract uses IERC721(collection).transferFrom(msg.sender, address(this), id); to transfer ERC721 tokens. This approach can lead to potential issues because transferFrom does not perform safety checks to ensure that the receiving contract is capable of handling ERC721 tokens.

Vulnerability Details:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible
Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Using safeTransferFrom instead of transferFrom is crucial for ERC721 transfers to ensure that the receiving contract implements the IERC721Receiver interface. This prevents tokens from being permanently locked in contracts that do not support ERC721 tokens.

Impact:

If the receiving contract (in this case, the escrow contract) does not implement the IERC721Receiver interface, the transferred tokens could be lost or locked, leading to a loss of assets for the users.

Tools Used:

Manual review

Recommendations

Replace IERC721(collection).transferFrom(msg.sender, address(this), id); with IERC721(collection).safeTransferFrom(msg.sender, address(this), id); to ensure that the receiving contract can handle ERC721 tokens properly. This change will invoke the onERC721Received function on the receiving contract, providing an additional layer of security.