Use safeTransferFrom() instead of transferFrom() for outgoing erc721 transfers

Summary

It is recommended to use safeTransferFrom() instead of transferFrom() when transferring ERC721s

Vulnerability Detail

The transferFrom() method is used instead of safeTransferFrom(), which I assume is a gas-saving measure. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(); use safeTransferFrom() whenever possible The recipient could have logic in the _initiateBridgeERC721() function, which is only triggered in the safeTransferFrom() function and not in transferFrom().

Impact

While unlikely because the recipient is the function caller, there is the potential loss for users should the recipient is unable to handle the sent ERC721s.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/L1ERC721Bridge.sol#L99-L101

        // Lock token into bridge
        deposits[_localToken][_remoteToken][_tokenId] = true;
        IERC721(_localToken).transferFrom(_from, address(this), _tokenId);

Tool used

Manual Review

Recommendation

Use safeTransferFrom() in the function initiateBridgeERC721()

- IERC721(_localToken).transferFrom(_from, address(this), _tokenId);
+ IERC721(_localToken).safetransferFrom(_from, address(this), _tokenId);

Reference

https://github.com/sherlock-audit/2022-09-harpie-judging#issue-m-1-use-safetransferfrom-instead-of-transferfrom-for-outgoing-erc721-transfers

sherlock-audit / 2023-01-optimism-judging

w42d3n - Use safeTransferFrom() instead of transferFrom() for outgoing erc721 transfers #299