First Flight #10: One Shot

Summary

The transferFrom() method is used instead of safeTransferFrom() in the contract, which is not recommended due to potential safety concerns.

Vulnerability Details

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

• https://github.com/Cyfrin/2024-02-one-shot/blob/main/src/RapBattle.sol#L46

• https://github.com/Cyfrin/2024-02-one-shot/blob/main/src/RapBattle.sol#L80

• https://github.com/Cyfrin/2024-02-one-shot/blob/main/src/Streets.sol#L34

• https://github.com/Cyfrin/2024-02-one-shot/blob/main/src/Streets.sol#L80

Impact

Using transferFrom() instead of safeTransferFrom() can lead to safety issues and unexpected behavior in certain NFTs that rely on the onERC721Received() function for additional logic.

Tools Used

Manual review

Recommendations

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. Note that the RapBattle contract should inherit the ERC721TokenReceiver contract as a consequence.