code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The usual business logic of the raffle should be that: If a user wins a raffle, he can always claim the NFT before a redraw can be initialized. However, the settings parameters can be set to inconsistent so that a winner may not be able to claim the NFT before a redraw can start.

Proof of Concept

In the test file: VRFNFTRandomDraw.t.sol, add the below test. The winner fails to get the NFT since the admin has claimed the NFT back.

    function test_WinnerNotAbleToClaimNFT() 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: 20 days,
                recoverTimelock: 10 days,
                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(15 days); //@audit - at this point, redraw cannot start; but admin claiming is allowed.

        drawing.lastResortTimelockOwnerClaimNFT(); //@audit - admin claim back the NFT

        assertTrue(drawing.hasUserWon(winner)); //@audit - the winner is till there

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

        vm.expectRevert("ERC721: transfer from incorrect owner");
        drawing.winnerClaimNFT();  //@audit - winner's claim failed.
    }

Tools Used

N/A

Recommended Mitigation Steps

Change parameter settings so that: Don't allow the admin to claim back NFT until redraw can be initialized.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #146