Forgeries contest - gz627's results

Lines of code

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

Vulnerability details

Impact

When initializing a draw, parameters in _settings can be easily set to inconsistent, which can lead to the winner cannot claim the NFT even before reaching the drawBufferTime – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.

Proof of Concept

Add the below test case in test file VRFNFTRandomDraw.t.sol. Time related seetings: drawBufferTime: 30 days, recoverTimelock: 2 weeks. In the test, when the winnder tries to claim NFT after 16 days after winning the game (when the drawBufferTime has not reached), the winner cannot get the NFT since the admin has legally claimed back the NFT with function call lastResortTimelockOwnerClaimNFT().

    function test_WinnerCannotClaimNFTBeforeDrawBufferTime() public {
        address winner = address(0x1337);
        vm.label(winner, "winner");

        vm.startPrank(winner);
        for (uint256 tokensCount = 0; tokensCount < 10; tokensCount++) {
            drawingNFT.mint();
        }
        vm.stopPrank();

        vm.startPrank(admin);
        targetNFT.mint();

        address consumerAddress = factory.makeNewDraw(
            IVRFNFTRandomDraw.Settings({
                token: address(targetNFT),
                tokenId: 0,
                drawingToken: address(drawingNFT),
                drawingTokenStartId: 0,
                drawingTokenEndId: 10,
                drawBufferTime: 30 days,
                recoverTimelock: 2 weeks,
                keyHash: bytes32(
                    0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15
                ),
                subscriptionId: subscriptionId
            })
        );
        vm.label(consumerAddress, "drawing instance");

        mockCoordinator.addConsumer(subscriptionId, consumerAddress);
        mockCoordinator.fundSubscription(subscriptionId, 100 ether);

        VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress);

        targetNFT.setApprovalForAll(consumerAddress, true);

        uint256 drawingId = drawing.startDraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);


        assertEq(targetNFT.balanceOf(winner), 0);
        assertEq(targetNFT.balanceOf(consumerAddress), 1);

        vm.warp(16 days);

        assertTrue(drawing.hasUserWon(winner));

        drawing.lastResortTimelockOwnerClaimNFT();

        assertTrue(drawing.hasUserWon(winner)); //@audit the winner does not change

        vm.stopPrank();
        vm.prank(winner);

        vm.expectRevert("ERC721: transfer from incorrect owner");
        // the winner can no longer claim the NFT as it has been claimed back by the admin
        drawing.winnerClaimNFT();
    }

Tools Used

Manual audit.

Recommended Mitigation Steps

Change the allowed range of drawing parameters. For example, only allow the winner to claim NFT within a pre-spedified time (say 1 day, 1 week, etc), and the admin can claim back NFT only after that time.