Forgeries contest - Ruhum's results

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.