Forgeries contest - poirots's results

Lines of code

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

Vulnerability details

Impact

Bad definition in L33: uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;, which is used to control settings in L86-L88 and L93-L98:

        if (
            _settings.recoverTimelock >
            block.timestamp + (MONTH_IN_SECONDS * 12)
        ) {
            revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
        }

        if (_settings.drawBufferTime > MONTH_IN_SECONDS) {
            revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();
        }

Consequences:

• One year as intended is in fact 7 years, which locks the owner in VRFNFTRandomDraw.sol#lastResortTimelockOwnerClaimNFT.
• Buffertime between redraws is 7 months, as checked in VRFNFTRandomDraw.sol#redraw().

External requirements:

• Badly configured settings

Proof of Concept

Check audit tags.

Redraw:

    function test_CreateWithBadMonthsRedraw() public {

        vm.startPrank(admin);
        targetNFT.mint();

        address consumerAddress = factory.makeNewDraw(
            IVRFNFTRandomDraw.Settings({
                token: address(targetNFT),
                tokenId: 0,
                drawingToken: address(drawingNFT),
                drawingTokenStartId: 0,
                drawingTokenEndId: 10,
                drawBufferTime: 20 weeks, //@audit change to 20 weeks (> 1 month)
                recoverTimelock: 50 weeks, //@audit change to 50 weeks (> 1 month)
                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);

        uint256 drawingId = drawing.startDraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);


        vm.warp(19 weeks);//@audit reverts after one month
        vm.expectRevert(IVRFNFTRandomDraw.TOO_SOON_TO_REDRAW.selector);
        drawing.redraw();
    }

Recommended Mitigation Steps

uint256 immutable MONTH_IN_SECONDS = 4 weeks;

QA Report

Low Risk Issues List

Total 5 issues

Non-Critical Issues List

Total 8 issues

[L-01] redraw() should be called by anyone

VRFNFTRandomDraw.sol#L203-L225

Redrawing a raffle is already protects the winner through the timelocking mechanism. Dependency on the owner should be avoidable in this instance by removing the modifier onlyOwner, allowing anyone to redraw the raffle.

[L-02] IERC721EnumerableUpgradeable may lead to false assumptions

Throughout the contract, there's a wrapper of NFT collections to IERC721EnumerableUpgradeable instances:

src/VRFNFTRandomDraw.sol:127:            IERC721EnumerableUpgradeable(_settings.token).ownerOf(
src/VRFNFTRandomDraw.sol:187:            IERC721EnumerableUpgradeable(settings.token).transferFrom(
src/VRFNFTRandomDraw.sol:216:            IERC721EnumerableUpgradeable(settings.token).ownerOf(
src/VRFNFTRandomDraw.sol:271:            IERC721EnumerableUpgradeable(settings.drawingToken).ownerOf(
src/VRFNFTRandomDraw.sol:295:        IERC721EnumerableUpgradeable(settings.token).transferFrom(
src/VRFNFTRandomDraw.sol:315:        IERC721EnumerableUpgradeable(settings.token).transferFrom(

This could lead to false assumptions when working with this contract (particularly when considering how settings are defined).

Consider altering to IERC721 if the goal is to allow any NFT collection compliant with EIP-721.

[L-03] drawingTokenEndId should be inclusive or altered to a range

Natspec specifies that the last ID is exclusive in the raffle, but the variable's name could lead to wrong assumptions.

Consider altering the logic to the contract to include the ID or to change the logic to a range definition, since it is only used twice (1,2) and could avoid misinterpretations.

[L-04] An owner can resign and lead to locked NFTs

Since there's a possibility of unclaimed drafts, the owner is the only one able to rescue the prize NFT from the raffle contract. Thus, having the ability to resign ownership (including non-intentional) could lead to stuck NFTs.

Consider altering or removing resignOwnership method:

    /// @notice Resign ownership of contract
    /// @dev only callably by the owner, dangerous call.
    function resignOwnership() public onlyOwner {
        _transferOwnership(address(0));
    }

[L-05] fulfillRandomWords must not revert

Accordingly to ChainLinks' documentation:

fulfillRandomWords must not revert If your fulfillRandomWords() implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert. Consider simply storing the randomness and taking more complex follow-on actions in separate contract calls made by you, your users, or an Automation Node.

This project current implementation does revert in two instances, although they are not expected to materialize.

Nevertheless, consider altering the logic to drop the random generated whenever the requestId does not match and ignore extra words if the array received is greater than the expected amount.

[N-01] getRequestDetails() should include the tokenid

In VRFNFTRandomDraw.sol#getRequestDetails() should include currentChosenTokenId (at) and ease integrations with other tools.

[N-02] Avoid setting time variables manually

Use solidity Time Units to avoid mistakes in defining time variables. In VRFNFTRandomDraw.sol#L28-L33 (the MONTH_IN_SECONDS leads to a medium issue):

/// @dev 60 seconds in a min, 60 mins in an hour
uint256 immutable HOUR_IN_SECONDS = 60 * 60;
/// @dev 24 hours in a day 7 days in a week
uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);
// @dev about 30 days in a month
uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

Consider changing to:

/// @dev 60 seconds in a min, 60 mins in an hour
uint256 immutable HOUR_IN_SECONDS = 1 hours;
/// @dev 24 hours in a day 7 days in a week
uint256 immutable WEEK_IN_SECONDS = 1 weeks;
// @dev about 30 days in a month
uint256 immutable MONTH_IN_SECONDS = 30 days;

[N-03] Use constants instead of immutable variables

Variables defined in VRFNFTRandomDraw.sol#L21-L33 should be constants, since they aren't define at contract creation:

    uint32 immutable callbackGasLimit = 200_000;
    /// @notice Chainlink request confirmations, left at the default
    uint16 immutable minimumRequestConfirmations = 3;
    /// @notice Number of words requested in a drawing
    uint16 immutable wordsRequested = 1;

    /// @dev 60 seconds in a min, 60 mins in an hour
    uint256 immutable HOUR_IN_SECONDS = 60 * 60;
    /// @dev 24 hours in a day 7 days in a week
    uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);
    // @dev about 30 days in a month
    uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

If these are rules, consider changing them to IVRFNFTRandomDraw interface:

    uint32 constant callbackGasLimit = 200_000;
    /// @notice Chainlink request confirmations, left at the default
    uint16 constant minimumRequestConfirmations = 3;
    /// @notice Number of words requested in a drawing
    uint16 constant wordsRequested = 1;

    /// @dev 60 seconds in a min, 60 mins in an hour
    uint256 constant HOUR_IN_SECONDS = 60 * 60;
    /// @dev 24 hours in a day 7 days in a week
    uint256 constant WEEK_IN_SECONDS = (3600 * 24 * 7);
    // @dev about 30 days in a month
    uint256 constant MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

[N-04] Uppercase immutable variables

In VRFNFTRandomDraw.sol#L22-L26:

    uint32 immutable callbackGasLimit = 200_000;
    /// @notice Chainlink request confirmations, left at the default
    uint16 immutable minimumRequestConfirmations = 3;
    /// @notice Number of words requested in a drawing
    uint16 immutable wordsRequested = 1;

In VRFNFTRandomDraw.sol#L37:

    VRFCoordinatorV2Interface immutable coordinator;

In Version.sol#L5:

  uint32 private immutable __version;

In VRFNFTRandomDrawFactory.sol#L21:

    address public immutable implementation;

[N-05] Empty blocks should be avoided

Avoid using code blocks, such as:

In VRFNFTRandomDrawFactory.sol#L53-L59:

    /// @notice Allows only the owner to upgrade the contract
    /// @param newImplementation proposed new upgrade implementation
    function _authorizeUpgrade(address newImplementation)
        internal
        override
        onlyOwner
    {}

Consider emiting an event.

[N-06] Missing NatSpec

Consider adding specification to the following code blocks:

In IVRFNFTRandomDraw.sol#L28:

    error REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();

[N-07] Contracts that extend interfaces should override its methods

Considering using the override keyword to indicate which methods are implementing the interface.

For VRFNFTRandomDraw regarding IVRFNFTRandomDraw: initialize, startDraw, redraw, hasUserWon, winnerClaimNFT, lastResortTimelockOwnerClaimNFT, getRequestDetails. For VRFNFTRandomDrawFactory regarding IVRFNFTRandomDrawFactory: initialize, startDraw.

[N-08] _requestRoll() after confirming that the raffle is viable

In startDraw(), the contract makes a request for a random number before confirming that it has the prize to raffle.

Consider confirming first that the contract has the NFT to raffle before wasting resources calling for a random.