Forgeries contest - trustindistrust's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L79-L85 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L102-L110 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L119-L125 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L162-L168

Vulnerability details

Description

The VRFNFTRandomDraw contract utilizes Chainlink VRF to provide randomness to the NFT raffle functionality.

In order to get randomness on demand, the contract supplies the Chainlink system a subscription ID, along with other information.

A subscription is funded with LINK. A subscription is also specific to a contract or set of contracts that are allowed to draw against it. LINK is deducted each time randomness is requested.

VRFNFTRandomDraw is also OwnableUpgradeable, which means the current owner can transfer control of the contract to a new owner.

However, the contract doesn't contain the ability to negate or forcefully update the subscription information upon transfer of ownership. As such, the new owner will continue to draw against the subscription when calling redraw() until the previous owner removes the contract from the subscription, or funds run out.

Complicating matters, if the previous owner does remember to remove the contract from his subscription, it causes the contract to cease functioning as a raffle. Any new attempts to redraw() after a user fails to claim will never change the value of request.currentChosenTokenId because the fulfillRandomWords will never be called by Chainlink. The new owner can only withdraw the NFT and deploy their own VRFNFTRandomDraw.

PoC

1. Alice creates a new VRFNFTRandomDraw by calling the factory contract normally, and supplying her subscription ID and keyhash. She then configures the subscription to accept calls from the raffle contract's address.
2. She then calls startDraw(), which transfers the NFT from her possession to the raffle.
3. For any particular reason, no users withdraw the NFT. Alice decides to transfer the ownership of the raffle to another associate Bob by calling VRFNFTRandomDraw.transferOwnership() with Bob's address.
4. After the drawBufferTime has elapsed, Bob calls redraw(). As this creates another request for randomness, Alice's subscription is billed for the request.
5. Alice notices, and removes the contract from her subscriptions.
6. Bob can now no longer run the raffle because the requestRandomWords function will revert. Bob is unable to update the subscription with his own information. The contract is now inert, and Bob can only withdraw the NFT, perhaps after a protracted delay.

The following Foundry test illustrates the issue:

function test_NewOwnerRafflesDrawDownSubscription() public {

    // Boilerplate setup steps
    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");

    mockCoordinator.addConsumer(subscriptionId, consumerAddress);
    mockCoordinator.fundSubscription(subscriptionId, 100 ether);
    VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress);
    targetNFT.setApprovalForAll(consumerAddress, true);
    // End boilerplate

    // Show the subscription balance before any actions taken
    (uint96 balanceBefore, , , ) = mockCoordinator.getSubscription(subscriptionId);
    console.log("Alice's subscription balance before startDraw(): %s", balanceBefore);

    // Kick off the raffle
    uint256 drawingId = drawing.startDraw();
    vm.stopPrank();

    mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);

    // Demonstrate that the subscription balance has declined
    (uint96 balanceAfter, , , ) = mockCoordinator.getSubscription(subscriptionId);
    console.log("Alice's subscription balance after startDraw(): %s", balanceAfter);

    // Transfer the raffle contract to Bob
    vm.startPrank(admin);
    drawing.transferOwnership(user);
    console.log("Owner Transferred to user");
    vm.stopPrank();

    // Redraw cooldown elapsed
    vm.warp(2 hours);

    // kick off new drawing
    vm.prank(user);
    uint256 newDrawing = drawing.redraw();

    mockCoordinator.fulfillRandomWords(newDrawing, consumerAddress);

    // Demonstrate alice's subscription was billed
    (uint96 balanceUser, , , ) = mockCoordinator.getSubscription(subscriptionId);
    console.log("Admin subscription balance after redraw(): %s", balanceUser);

    // Alice removes the subscription for the raffle contract
    vm.prank(admin);
    mockCoordinator.removeConsumer(subscriptionId, consumerAddress);

    vm.warp(4 hours);

    // Bob tries again, now this reverts
    vm.prank(user);
    vm.expectRevert(VRFCoordinatorV2Mock.InvalidConsumer.selector);
    drawing.redraw();

    vm.warp(2 weeks);

    // Unable to do anything else, Bob claims the NFT
    vm.prank(user);
    drawing.lastResortTimelockOwnerClaimNFT();
    assertEq(targetNFT.balanceOf(user), 1);
}

Severity Rationalization

As this situation causes a drain on the LINK of the initial owner of the VRFNFTRandomDraw contract, and further could be fairly impactful based on circumstances (owner doesn't notice or remember to change the subscription, while the raffle goes on for a protracted period of time), this would seem to qualify as a High severity issue. The initial owner must remember to do the right thing before redraw() can be called.

Mitigation

The most straightforward fix for this is to simply remove the ownership update functionality, and provide documentation indicating that the correct course of action is to re-deploy the VRFNFTRandomDraw contract with the new owner, who can then provide their Chainlink information.

Since the ownership change functionality results in a contract that doesn't function properly when the initial owner removes the contract from their subscription, it's unclear why the contract's owner can be changed.

If the ownership migration is deemed desirable enough to keep, then the functionality needs to be updated to allow for updating the IVRFNFTRandomDraw.Settings struct for the contract. This could be done on acceptOwnership() with parameters checked against the current values (so they can't just be re-used). Otherwise, new functions would have to be added to VRFNFTRandomDraw to allow updating the struct.

Tools Used

Foundry, manual review