Forgeries contest - csanuragjain's results

Lines of code

https://github.com/0xigami/vrf-nft-raffle/blob/main/src/VRFNFTRandomDraw.sol#L304

Vulnerability details

Impact

It was observed that if a winner is chosen and request.drawTimelock is still remaining, still Admin can withdraw the NFT. This will cause winner to get nothing

Proof of Concept

1. Admin has started new draw using startDraw function. Lets say settings.recoverTimelock is X

function startDraw() external onlyOwner returns (uint256) {
        // Only can be called on first drawing
        if (request.currentChainlinkRequestId != 0) {
            revert REQUEST_IN_FLIGHT();
        }

        // Emit setup draw user event
        emit SetupDraw(msg.sender, settings);

        // Request initial roll
        _requestRoll();

        // Attempt to transfer token into this address
        try
            IERC721EnumerableUpgradeable(settings.token).transferFrom(
                msg.sender,
                address(this),
                settings.tokenId
            )
        {} catch {
            revert TOKEN_NEEDS_TO_BE_APPROVED_TO_CONTRACT();
        }

        // Return the current chainlink request id
        return request.currentChainlinkRequestId;
    }

2. A winner is chosen by chainlink using the fulfillRandomWords function

3. Assume User A has won

4. settings.recoverTimelock (X) time has passed, but request.drawTimelock is still left

5. Admin simply calls lastResortTimelockOwnerClaimNFT function which transfer the NFT back to Admin even though request.drawTimelock is not reached and winner was already present

function lastResortTimelockOwnerClaimNFT() external onlyOwner {
        // If recoverTimelock is not setup, or if not yet occurred
        if (settings.recoverTimelock > block.timestamp) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
        }

        // Send event for indexing that the owner reclaimed the NFT
        emit OwnerReclaimedNFT(owner());

        // Transfer token to the admin/owner.
        IERC721EnumerableUpgradeable(settings.token).transferFrom(
            address(this),
            owner(),
            settings.tokenId
        );
    }

Recommended Mitigation Steps

Admin should not be allowed to withdraw if request.drawTimelock is not over.

function lastResortTimelockOwnerClaimNFT() external onlyOwner {
require(request.drawTimelock < block.timestamp && request.hasChosenRandomNumber, "Cannot withdraw now");
...
}