code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L187

Vulnerability details

Vulnerability Detail

OpenZeppelin’s documentation discourages the use of transferFrom(); use safeTransferFrom() whenever possible
The recipient could have logic in the startDraw(), fwinnerClaimNFT(), lastResortTimelockOwnerClaimNFT(), function

src/VRFNFTRandomDraw.sol-185-        // Attempt to transfer token into this address
src/VRFNFTRandomDraw.sol-186-        try
src/VRFNFTRandomDraw.sol:187:            IERC721EnumerableUpgradeable(settings.token).transferFrom(
src/VRFNFTRandomDraw.sol-188-                msg.sender,
src/VRFNFTRandomDraw.sol-189-                address(this),
--
src/VRFNFTRandomDraw.sol-293-
src/VRFNFTRandomDraw.sol-294-        // Transfer token to the winter.
src/VRFNFTRandomDraw.sol:295:        IERC721EnumerableUpgradeable(settings.token).transferFrom(
src/VRFNFTRandomDraw.sol-296-            address(this),
src/VRFNFTRandomDraw.sol-297-            msg.sender,
--
src/VRFNFTRandomDraw.sol-313-
src/VRFNFTRandomDraw.sol-314-        // Transfer token to the admin/owner.
src/VRFNFTRandomDraw.sol:315:        IERC721EnumerableUpgradeable(settings.token).transferFrom(
src/VRFNFTRandomDraw.sol-316-            address(this),
src/VRFNFTRandomDraw.sol-317-            owner(),

It helps ensure that the recipient is indeed capable of handling ERC721s.

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.

Code Snippet

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L187

Recommendation

Use safeTransferFrom()

- IERC721EnumerableUpgradeable(settings.token).transferFrom(
+ IERC721EnumerableUpgradeable(settings.token).safetransferFrom(

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #356