Forgeries contest - immeas's results

Lines of code

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

Vulnerability details

Description

When creating a random draw the owner specifices a recoverTimelock which is a last resort option to recover the raffled NFT if the draw fails.

There are some validations that this is between a week and a year in the future but there's no guarantee that the draw actually will start before the recoverTimeLock is possible.

Impact

A malicious owner could wait with starting the draw until after recoverTimeLock and thus affect the outcome of the draw by withdrawing the NFT before the winner can claim it.

This could affect trust in using this protocol for random draws as the winner is not guaranteed a chance to claim their NFT.

Eve holds a lottery of a fancy ArtGobbler for her two friends Alice and Bob. She mints two NFT lottery tickets for 10 eth each. Eve opens the random draw but waits with starting the draw until after recoverTimeLock has passed. Then when the result comes in and one of them wins Eve quickly calls lastResortTimelockOwnerClaimNFT to take back the NFT and keep the funds.

Proof of Concept

PoC test in VRFNFTRandomDraw.t.sol:

    function test_FullDrawingOwnerReclaimsNft() 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: block.timestamp + 1 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 until after recoverTimeLock to start the drawing
        vm.warp(block.timestamp + 1 weeks);
        uint256 drawingId = drawing.startDraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
        
        // admin can now stop if the wrong user won for example
        drawing.lastResortTimelockOwnerClaimNFT();
        vm.stopPrank();
        
        vm.prank(winner);
        vm.expectRevert();
        // no nft to win
        drawing.winnerClaimNFT();
        assertEq(targetNFT.balanceOf(winner), 0);
        assertEq(targetNFT.balanceOf(consumerAddress), 0); 
        assertEq(targetNFT.balanceOf(admin), 1); // admin has nft
    }

Tools Used

manual review, forge

Recommended Mitigation Steps

instead of providing an absolute timestamp in recoverTimeLock have it as a recoverTime which is set when the NFT is transferred to the contract (startDraw).

That way the user knows that the NFT will be available to claim for a certain time once the draw starts.

low

L-1 draw can be started with redraw() instead of startDraw()

By transfering the raffled NFT to the contract manually a owner can start the raffle with redraw() instead of startDraw().

Impact

This omits sending the SetupDraw event which could affect listeners to the protocol. It also breaks the intended usage flow.

Proof of Concept

PoC test in VRFNFTRandomDraw.t.sol:

   function test_FullDrawingStartWithRedraw() 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: block.timestamp + 1 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);

        // move nft to contract manually
        targetNFT.transferFrom(admin, consumerAddress, 0); 
        // start with redraw instead of startDraw()
        uint256 drawingId = drawing.redraw();

        mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
        vm.stopPrank();
        
        vm.prank(winner);
        drawing.winnerClaimNFT();
        assertEq(targetNFT.balanceOf(winner), 1);
        assertEq(targetNFT.balanceOf(consumerAddress), 0); 
    }

Tools Used

vs code, forge

Recommended Mitigation Steps

before doing a redraw(), verify that currentChainlinkRequestId is not empty. Then has to have been started before (through startDraw()).

L-2 weird comment

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/interfaces/IVRFNFTRandomDraw.sol#L64

File: interfaces/IVRFNFTRandomDraw.sol

64:        /// @notice has chosen a random number (in case random number = 0(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0))

L-3 typos in comments

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/interfaces/IVRFNFTRandomDraw.sol#L46

File: interfaces/IVRFNFTRandomDraw.sol

46:    /// @notice When the owner reclaims nft aftr recovery time delay

aftr

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

File: VRFNFTRandomDraw.sol

294:        // Transfer token to the winter.

winter

L-4 anyone can call initialize()

There is no validation that it's the factory that created the contract that is calling initialize(). This could be achieved by adding immutable value with the creator in the constructor.

non-crit

NC-1 unreachable code #1

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

In _requestRoll():

141:    function _requestRoll() internal {
142:        // Chainlink request cannot be currently in flight.
143:        // Request is cleared in re-roll if conditions are correct.
144:        if (request.currentChainlinkRequestId != 0) { // can never be !=0
145:            revert REQUEST_IN_FLIGHT();
146:        }

This can never happen. _requestRoll() is called from two places:

startDraw() which checks that currentChainlinkRequestId != 0 before going into _requestRoll():

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

...

182:        // Request initial roll
183:        _requestRoll();

and in redraw() which just before does delete request which resets currentChainlinkRequestId to 0:

203:    function redraw() external onlyOwner returns (uint256) {
204:        if (request.drawTimelock >= block.timestamp) {
205:            revert TOO_SOON_TO_REDRAW();
206:        }
207:
208:        // Reset request
209:        delete request;
210:
211:        // Re-roll
212:        _requestRoll();

NC-2 unreachable code #2

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

In _requestRoll():

203:        if (
204:            request.hasChosenRandomNumber &&
205:            // Draw timelock not yet used
206:            request.drawTimelock != 0 &&
207:            request.drawTimelock > block.timestamp
208:        ) {
209:            revert STILL_IN_WAITING_PERIOD_BEFORE_REDRAWING();
210:        }

This can never happen. _requestRoll() is called from these two places:

startDraw(), already has a check for that hasChosenRandomNumber is 0:

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

...

182:        // Request initial roll
183:        _requestRoll();

and in redraw() request.drawTimelock is checked already and it then does delete request which resets currentChainlinkRequestId to 0:

203:    function redraw() external onlyOwner returns (uint256) {
204:        if (request.drawTimelock >= block.timestamp) {
205:            revert TOO_SOON_TO_REDRAW();
206:        }
207:
208:        // Reset request
209:        delete request;
210:
211:        // Re-roll
212:        _requestRoll();