DittoETH

Summary

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

Vulnerability Details

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 onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom(). A notable example of such contracts is the Sudoswap pair:

NOTE: Although it is written in the comment that "the caller is responsible to confirm that the recipient is capable of receiving ERC721

 * `or else they may be permanently lost. Usage of {safeTransferFrom} prevents loss, though the caller must`

 * `understand this adds an external call which potentially creates a reentrancy vulnerability."`

But this issue can be easily avoided by adding a re-entrancy guard so it is better to use safeTransferFrom with re-entrancy guard and avoid this issue

Impact

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

Tools Used

Manual Review

Recommendations

Use safeTransferFrom() when sending out the NFT.