ERC20 can be mistakenly used instead of ERC721

[code423n4]

Lines of code

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

Vulnerability details

Impact

// Attempt to transfer token into this address
try
    IERC721EnumerableUpgradeable(settings.token).transferFrom( 
        // @audit could use ERC20 here
        msg.sender,
        address(this),
        settings.tokenId
    )
{} catch {
    revert TOKEN_NEEDS_TO_BE_APPROVED_TO_CONTRACT();
}

Both ERC20 and ERC721 has the common function transferFrom(), it also shares the same set of input params (address, address, uint256).

So in this case, ERC20 token contract can be mistakenly used as NFT to be drawn.

However, this ERC20 token cannot be claimed by winner and will be locked in the contract. Because, in function hasUserWon(), it does a check using ownerOf() function which did not exist in ERC20 contract. ERC20 contract will revert, resulting in fail claiming.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider checking contract using supportInterface() instead of only checking code length.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[gzeoneth]

This is not possible as the following would reverted during init https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L127-L129