Forgeries contest - dipp's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L90-L98 https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L304-L320

Vulnerability details

Impact

When a VRFNFTRandomDraw contract is initialized, the recoverTimelock variable is set. The variable should be used to prevent the admin from calling the lastResortTimelockOwnerClaimNFT function before a certain amount of time has passed to ensure that the NFT is being raffled fairly and the winner has enough time to claim it.

Since the variable is set in the initialize function, an admin could wait until the timelock has passed before calling the startDraw function to start the raffle so that lastResortTimelockOwnerClaimNFT is callable immediately. Users participating in the raffle would not expect the admin to be able to withdraw so soon after starting the raffle.

This could be used by malicious admins to trick users into entering a raffle, with the expectation that they would have at least 1 week to claim the NFT, and then withdrawing the NFT before the winner claims it.

Proof of Concept

1. An admin initializes a VRFNFTRandomDraw contract with the recoverTimelock set to 2 weeks from intitializaion.

2. The admin waits 2 weeks then starts the raffle by calling startDraw.

3. Immediately after a winner is selected the admin calls lastResortTimelockOwnerClaimNFT to withdraw the NFT before the winner is able to claim it.

The test below shows the admin calling startDraw once the recoverTimelock has passed and the withdrawing the NFT immediately after a winner has been chosen.

Test added to VRFNFTRandomDraw.t.sol:

    function test_AdminWithdrawsUnexpectedly() 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: 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);

        // Admin waits 2 weeks for the recoverTimelock to pass before calling startDraw
        vm.warp(2 weeks);

        uint256 drawingId = drawing.startDraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);

        // Admin is able to withdraw NFT before winner
        drawing.lastResortTimelockOwnerClaimNFT();

        vm.stopPrank();

        // Winner cannot claim NFT
        vm.startPrank(winner);
        vm.expectRevert();
        drawing.winnerClaimNFT();
        vm.stopPrank();

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

Tools Used

Recommended Mitigation Steps

Consider setting the recoverTimelock in the startDraw function so that users always have at least 1 week to withdraw their NFTs. This could be done by storing the amount of time until an admin can recover an NFT in the settings variable similar to how drawBufferTime is stored in settings instead of drawTimelock.