Chosen drawing tokenIds range can be out of range.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Drawing token range can be out of range for existing tokenIds for a collection. This would lead to raffle never finding a suitable winner to claim the prize.

This also opens the door for silent bigotry from admin who could use only token ranges that are out of existing tokenIds. admin would then claim no winner's chose to claim the prize and then he calls lastResortTimelockOwnerClaimNFT().

Proof of Concept

Admin advertises upcoming raffle for everyone that buys from X NFT collection. People buy a bunch X NFT collection. admin chooses range that doesnt reflect existing or possible number of tokenIds in collection. No one truly wins, admin withdraws prize.

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

        if (
            _settings.drawingTokenEndId < _settings.drawingTokenStartId ||
            _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2
        ) {
            revert DRAWING_TOKEN_RANGE_INVALID();
        }

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

    function lastResortTimelockOwnerClaimNFT() external onlyOwner {
        // If recoverTimelock is not setup, or if not yet occurred
        if (settings.recoverTimelock > block.timestamp) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
        }

        // Send event for indexing that the owner reclaimed the NFT
        emit OwnerReclaimedNFT(owner());

        // Transfer token to the admin/owner.
        IERC721EnumerableUpgradeable(settings.token).transferFrom(
            address(this),
            owner(),
            settings.tokenId
        );
    }

Recommended Mitigation Steps

Check tokenIds in range exist by calling tokenURI() and making sure it doesn't revert.

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-c