code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description

The VRFNFTRandomDraw contract offers two draw functions: startDraw() and redraw(). It is assumed that every draw starts with startDraw, and then extended using redraw() until a winner is finally selected. However, it turns out that startDraw() can be bypassed and redraw() called directly after contract initialization.

function redraw() external onlyOwner returns (uint256) {
    if (request.drawTimelock >= block.timestamp) {
        revert TOO_SOON_TO_REDRAW();
    }
    // Reset request
    delete request;
    // Re-roll
    _requestRoll();
    // Owner of token to raffle needs to be this contract
    if (
        IERC721EnumerableUpgradeable(settings.token).ownerOf(
            settings.tokenId
        ) != address(this)
    ) {
        revert DOES_NOT_OWN_NFT();
    }
    // Return current chainlink request ID
    return request.currentChainlinkRequestId;
}

After initialization, the drawTimelock check will always pass because it's zero. Request is deleted. Therefore, all checks in _requestRoll() are nullified:

function _requestRoll() internal {
    // Chainlink request cannot be currently in flight.
    // Request is cleared in re-roll if conditions are correct.
        // <--------------------------- will be 0 ----->
    if (request.currentChainlinkRequestId != 0) {
        revert REQUEST_IN_FLIGHT();
    }
    // If the number has been drawn and
    if (
        //  <--------------------------- will be 0 ----->
        request.hasChosenRandomNumber &&
        // Draw timelock not yet used
        request.drawTimelock != 0 &&
        request.drawTimelock > block.timestamp
    ) {
        revert STILL_IN_WAITING_PERIOD_BEFORE_REDRAWING();
    }
    // Setup re-draw timelock
    request.drawTimelock = block.timestamp + settings.drawBufferTime;
    // Request first random round
    request.currentChainlinkRequestId = coordinator.requestRandomWords({
        keyHash: settings.keyHash,
        subId: settings.subscriptionId,
        minimumRequestConfirmations: minimumRequestConfirmations,
        callbackGasLimit: callbackGasLimit,
        numWords: wordsRequested
    });
}

Note that redraw() doesn't transfer the NFT to the contract. Host will need to transfer it because it checks that the owner of the NFT is address(this).

Without startDraw() this event will never emit: emit SetupDraw(msg.sender, settings);. This may interfere with the platform in unexpected ways.

Impact

Draws can be initiated without startDraw() call, which can cause confusion and state mismatch.

Tools Used

Manual audit

Recommended Mitigation Steps

Please add this check in redraw():

if (request.currentChainlinkRequestId == 0) {
            revert REQUEST_IN_FLIGHT();
}

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)

gzeoneth commented 1 year ago

no functionality loss, event and state handling