code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

If a user adds new consumer, function VRFCoordinatorV2::addConsumer is called:

  function addConsumer(uint64 subId, address consumer) external override onlySubOwner(subId) nonReentrant {
    // Already maxed, cannot add any more consumers.
    if (s_subscriptionConfigs[subId].consumers.length == MAX_CONSUMERS) {
      revert TooManyConsumers();
    }

It reverts if s_subscriptionConfigs[subId].consumers.length == MAX_CONSUMERS - MAX_CONSUMERS is hardcoded to 100. Hence, one subscriptionID may have only 100 consumers. User creating a raffle passes subscriptionID in settings during contract creation, and are unable to change it afterwards. They will only know that they are not whitelisted (because Chainlink's smart contract does not allow to do that) only when trying to start a raffle.

Proof of Concept

Please refer to test that ilustrate this issue:

    function test_SubscriptionConsumerOverflow() public {
        address sender = address(0x994);
        vm.label(sender, "sender");

        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);

        mockCoordinator.fundSubscription(subscriptionId, 100 ether);

        for (uint256 i; i < 100; ++i) {
            mockCoordinator.addConsumer(subscriptionId, address(uint160(i + 1)));
        }

        vm.stopPrank();

        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: 2 weeks,
                keyHash: bytes32(
                    0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15
                ),
                subscriptionId: subscriptionId
            })
        );
        vm.label(consumerAddress, "drawing instance");

        targetNFT.setApprovalForAll(consumerAddress, true);

        //it reverts, subscriptionLimit reached, but the smart contract is already created
        vm.expectRevert(VRFCoordinatorV2.TooManyConsumers.selector);
        mockCoordinator.addConsumer(subscriptionId, address(consumerAddress));

        VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress);

        //startDraw => fulfillRandomWords cannot be called, as consumer is not subscribed
        //hence, it reverts, and user has no chance to change subscriptionID
        uint256 drawingId = drawing.startDraw();

        vm.stopPrank();
    }

Tools Used

VS Code, foundry

Recommended Mitigation Steps

Allow owner to change subscriptionID by creating specific function.

diff --git a/src/VRFNFTRandomDraw.sol b/src/VRFNFTRandomDraw.sol
index 668bc56..d62c95a 100644
--- a/src/VRFNFTRandomDraw.sol
+++ b/src/VRFNFTRandomDraw.sol
@@ -168,6 +168,10 @@ contract VRFNFTRandomDraw is
         });
     }

+    function setSubscriptionId(uint256 _subscriptionId) external onlyOwner {
+        settings.subscriptionId = _subscriptionId;
+    }
+
     /// @notice Call this to start the raffle drawing
     /// @return chainlink request id
     function startDraw() external onlyOwner returns (uint256) {

zobront commented 1 year ago

This is a good find but subscriptionId is passed into the settings when new Clones are created so it doesn't seem there is any DOS risk.