CryptoPunks are not compatible with VRFNFTRandomDraw

[code423n4]

Lines of code

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

Vulnerability details

Impact

Legacy NFTs like CryptoPunks don't follow the ERC721 standard. Those tokens won't be usable as either drawing or raffled tokens. Because the contest documentation explicitly stated the usage of CryptoPunks as drawing tokens, I thought it's worth bringing up here.

We want to raffle away a single NFT (token) based off of another NFT collection (or drawingToken) in a fair and trustless manner. For instance, we could raffle off a single high value NFT to any cryptopunk holder, the punk that wins can choose to claim the NFT. If they do not claim, a re-roll or redraw can be done to select a new holder that would be able to claim the NFT.

This does not result in any loss of funds. It only impacts the functionality of the contract.

Proof of Concept

The VRFNFTRandomDraw expects the drawingToken to have an ownerOf() function. Calls to that function will revert resulting in nobody being able to claim their NFT.

    function hasUserWon(address user) public view returns (bool) {
        if (!request.hasChosenRandomNumber) {
            revert NEEDS_TO_HAVE_CHOSEN_A_NUMBER();
        }

        return
            user ==
            IERC721EnumerableUpgradeable(settings.drawingToken).ownerOf(
                request.currentChosenTokenId
            );
    }

As seen here, the CryptoPunks contract doesn't have an ownerOf() function. Instead, you have to use punkIndexToAddress().

Tools Used

none

Recommended Mitigation Steps

You can write a custom adapter that acts as an in-between for the VRFNFTRandomDraw and legacy NFT contracts.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[trust1995]

Cryptopunk compatibility is submitted often in ERC721-centric protocols. From what I've seen, they are considered QA, such as in PartyDAO , Cally and recently by Alex in Blur. They can always be externally wrapped which is what mostly happens in these types of protocols, without the requirement of the project to do it. It's worth standardizing this type of submission, maybe alongside other popular non-ERC20s. Will open a DAO discussion.

[iainnash]

Would argue that this is not an issue. It's an example of a known contract that isn't an NFT and we require a valid NFT to support this protocol. Additionally, this is not mentioned anywhere in the code and in my opinion should fail which is the correct behavior in this case.

[iainnash]

I would say most people use wrapped punks in situations such as this — as @trust1995 has written

[c4-sponsor]

iainnash marked the issue as sponsor disputed

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b