Drawing with invalid subscriptionId is useless

[code423n4]

Lines of code

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

Vulnerability details

Impact

When a user creates a drawing, he must specify a subscriptionId, which is then used for Chainlink VRF. However, it is never checked if the subscription is valid. Also, it can never be changed. So if the user inputs the wrong ID, the winner can never be drawn.

Proof Of Concept

Add this code to test/VRFNFTRandomDraw.t.sol. The test will fail with InvalidConsumer() when startDraw is called. Notice that this is the error that is returned by VRFCoordinatorV2Mock.sol which is a bit different than the original VRFCoordinatorV2.sol. The original contract would revert with InvalidSubscription().

function test_WrongSubscription() public {
        vm.startPrank(admin);
        targetNFT.mint();
    
        vm.expectRevert();
        address consumerAddress = factory.makeNewDraw(
            IVRFNFTRandomDraw.Settings({
                token: address(targetNFT),
                tokenId: 0,
                drawingToken: address(drawingNFT),
                drawingTokenStartId: 0,
                drawingTokenEndId: 10,
                drawBufferTime: 1 hours,
                recoverTimelock: 2 weeks,
                keyHash: bytes32(
                    0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15
                ),
                subscriptionId: 1234
            })
        );
        vm.label(consumerAddress, "drawing instance");

        VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress);

        targetNFT.setApprovalForAll(consumerAddress, true);

        uint256 drawingId = drawing.startDraw();
    }

The above code is just to show what the problem is and it is not a proper test that you would include in your tests. Instead if you decide to fix this problem, I recommend that you add a test like this one:

function test_WrongSubscription() public {
        vm.startPrank(admin);
        targetNFT.mint();
    
        vm.expectRevert();
        address consumerAddress = factory.makeNewDraw(
            IVRFNFTRandomDraw.Settings({
                token: address(targetNFT),
                tokenId: 0,
                drawingToken: address(drawingNFT),
                drawingTokenStartId: 0,
                drawingTokenEndId: 10,
                drawBufferTime: 1 hours,
                recoverTimelock: 2 weeks,
                keyHash: bytes32(
                    0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15
                ),
                subscriptionId: 1234 // make sure that whatever value you put here is not a valid subscription
            })
        );
    }

Recommended Mitigation Steps

Add subscriptionId validation to initialize function in VRFNFTRandomDraw.sol. You can validate it by calling VRFCoordinatorV2.getSubscription(subId) and checking that the owner address is equal to the creator (or some other address that should be responsible for the VRF subscription).

(, , address subscriptionOwner,) = coordinator.getSubscitption(_settings.subscriptionId);
if(subscriptionOwner != admin) revert WRONG_SUBSCRIPTION_ID();

[c4-judge]

gzeon-c4 marked the issue as duplicate of #194

[c4-judge]

gzeon-c4 marked the issue as satisfactory