search

code-423n4 / 2022-12-forgeries-findings

0 stars 0 forks source link

safeTransfer should be used instead of transferFrom in `winnerClaimNFT` #326

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The winner of the raffle may be a smart contract which doesn't handle NFTs, thus leaving the NFT irretrievably lost instead of being awarded to an address which can handle NFTs. Additionally, the NFT should be in the VRFNFTRandomDraw contract at the time of the claiming. There is no need to use transferFrom in winnerClaimNFT or lastResortTimelockOwnerClaimNFT - safeTransfer / transfer is enough to transfer it out of the contract.

Proof of Concept

Deploy a standard contract with no ERC721 support, send one of the drawingToken NFTs to it. Deploy a raffle using makeNewDraw in VRFNFTRandomDrawFactory, using the drawingToken as the NFT collection for the raffle. Let's say the contract is selected as winner in startDraw and calls winnerClaimNFT. The NFT gets transferred to the contract without checking if the contract is a ERC721Receiver, rendering the NFT lost.

Tools Used

VS Code, Forge

Recommended Mitigation Steps

Use safeTransfer instead of transferFrom in winnerClaimNFT.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #356

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Out of scope