Art Gobblers contest - zkhorse's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L558-L567

Vulnerability details

The owner of the ArtGobblers contract has the authority to upgrade the randomness provider, and a comment notes that this functionality may be "useful if current VRF is sunset". However, if the owner attempts to update the randomness provider while there is a pending request for randomness tracked in gobblerRevealsData.waitingForSeed, the call will revert with a SeedPending error:

ArtGobblers#upgradeRandProvider

    /// @notice Upgrade the rand provider contract. Useful if current VRF is sunset.
    /// @param newRandProvider The new randomness provider contract address.
    function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
        // Revert if waiting for seed, so we don't interrupt requests in flight.
        if (gobblerRevealsData.waitingForSeed) revert SeedPending();

        randProvider = newRandProvider; // Update the randomness provider.

        emit RandProviderUpgraded(msg.sender, newRandProvider);
    }

Gobbler reveals are locked while the seed is pending. Only a callback from the current randomness provider to acceptRandomSeed can clear waitingForSeed and unlock reveals:

ArtGobblers#acceptRandomSeed

    /// @notice Callback from rand provider. Sets randomSeed. Can only be called by the rand provider.
    /// @param randomness The 256 bits of verifiable randomness provided by the rand provider.
    function acceptRandomSeed(bytes32, uint256 randomness) external {
        // The caller must be the randomness provider, revert in the case it's not.
        if (msg.sender != address(randProvider)) revert NotRandProvider();

        // The unchecked cast to uint64 is equivalent to moduloing the randomness by 2**64.
        gobblerRevealsData.randomSeed = uint64(randomness); // 64 bits of randomness is plenty.

        gobblerRevealsData.waitingForSeed = false; // We have the seed now, open up reveals.

        emit RandomnessFulfilled(randomness);
    }

Although admittedly very unlikely, it's possible that reveals may be permanently blocked if the acceptRandomSeed callback never arrives from the randomness provider:

• Call ArtGobblers#requestRandomSeed, which sets waitingForSeed to true.
• An act of God smites the randomness provider, so acceptRandomSeed is never called.
• Since the seed will be permanently pending, it is impossible to call upgradeRandProvider and replace it.

Impact

Gobbler reveals will be permanently bricked. We consider this extremely low likelihood, but very high impact, so we are reporting it as a Medium.

Recommendation

Consider tracking the last revealed timestamp and adding a timeout to the upgrade logic. (Or potentially, using the already stored nextRevealTimestamp to calculate a timeout). If this timeout expires and the randomness provider callback has not been received, allow the contract owner to change the provider.

Low

ArtGobblers and Pages: Token mints do not use safe transfers

Functions that mint tokens in ArtGobblers and Pages (ArtGobblers#claimGobbler, ArtGobblers#mintFromGoo, ArtGobblers#mintLegendaryGobbler, Pages#mintFromGoo, Pages#mintCommunityPages) do not check that the recipient address supports ERC721 token transfers. If this is a concern, consider using _safeMint to mint tokens.

QA

ArtGobblers: Zero goo amounts may be added/removed from balances

It's possible to call addGoo and removeGoo with a zero gooAmount:

ArtGobblers#L767-L787

    /// @notice Add goo to your emission balance,
    /// burning the corresponding ERC20 balance.
    /// @param gooAmount The amount of goo to add.
    function addGoo(uint256 gooAmount) external {
        // Burn goo being added to gobbler.
        goo.burnForGobblers(msg.sender, gooAmount);

        // Increase msg.sender's virtual goo balance.
        updateUserGooBalance(msg.sender, gooAmount, GooBalanceUpdateType.INCREASE);
    }

    /// @notice Remove goo from your emission balance, and
    /// add the corresponding amount to your ERC20 balance.
    /// @param gooAmount The amount of goo to remove.
    function removeGoo(uint256 gooAmount) external {
        // Decrease msg.sender's virtual goo balance.
        updateUserGooBalance(msg.sender, gooAmount, GooBalanceUpdateType.DECREASE);

        // Mint the corresponding amount of ERC20 goo.
        goo.mintForGobblers(msg.sender, gooAmount);
    }

This will make an internal call to updateUsergooBalance and emit a GooBalanceUpdated event.

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L809-L829

    /// @notice Update a user's virtual goo balance.
    /// @param user The user whose virtual goo balance we should update.
    /// @param gooAmount The amount of goo to update the user's virtual balance by.
    /// @param updateType Whether to increase or decrease the user's balance by gooAmount.
    function updateUserGooBalance(
        address user,
        uint256 gooAmount,
        GooBalanceUpdateType updateType
    ) internal {
        // Will revert due to underflow if we're decreasing by more than the user's current balance.
        // Don't need to do checked addition in the increase case, but we do it anyway for convenience.
        uint256 updatedBalance = updateType == GooBalanceUpdateType.INCREASE
            ? gooBalance(user) + gooAmount
            : gooBalance(user) - gooAmount;

        // Snapshot the user's new goo balance with the current timestamp.
        getUserData[user].lastBalance = uint128(updatedBalance);
        getUserData[user].lastTimestamp = uint64(block.timestamp);

        emit GooBalanceUpdated(user, updatedBalance);
    }

Since this wastes gas for the caller and emits a confusing event, consider checking for zero amounts in addGoo and removeGoo.

Additionally, we found that calling addGoo(0) repeatedly and frequently at intervals of less than 1 day can cause a user's goo balance to be lower than expected. We didn't see a clear path to an exploit here, since the error penalizes rather than rewards the caller, but it is unusual behavior that may deserve a closer look. See the test below, which passes with TIMESTEP_OK and fails with TIMESTEP_FAIL.

    /// @notice make sure that actions that trigger balance snapshotting do not affect total balance.
    function testSnapshotDoesNotAffectBalanceFuzzed() public {
        //mint one gobbler for each user
        mintGobblerToAddress(users[0], 1);
        mintGobblerToAddress(users[1], 1);
        vm.warp(block.timestamp + 1 days);
        //give user initial goo balance
        vm.prank(address(gobblers));
        goo.mintForGobblers(users[0], 100);
        //reveal gobblers
        bytes32 requestId = gobblers.requestRandomSeed();
        uint256 randomness = 1022; // magic seed to ensure both gobblers have same multiplier
        vrfCoordinator.callBackWithRandomness(requestId, randomness, address(randProvider));
        gobblers.revealGobblers(2);
        //make sure both gobblers have same multiple, and same starting balance
        assertGt(gobblers.getUserEmissionMultiple(users[0]), 0);
        assertEq(gobblers.getUserEmissionMultiple(users[0]), gobblers.getUserEmissionMultiple(users[1]));

        assertEq(gobblers.gooBalance(users[0]), gobblers.gooBalance(users[1]));

        uint256 TIMESTEP_OK = 1 days;
        uint256 TIMESTEP_FAIL = 1 days - 1;
        unchecked {
            for (uint256 n = 0; n < 1000; n += 12) {
                vm.warp(block.timestamp + TIMESTEP_OK);
                vm.prank(users[0]);
                gobblers.addGoo(0);
                console.log("balance1 - balance0 =", gobblers.gooBalance(users[1]) - gobblers.gooBalance(users[0]));
            }
        }

        // make sure users have equal balance
        unchecked {
            assertEq(0,  gobblers.gooBalance(users[1]) - gobblers.gooBalance(users[0]));
        }
    }

ArtGobblers: Gobbling ignores token approvals

The caller of ArtGobblers#gobble must be the owner of the Gobbler being fed:

ArtGobblers#gobble

        // The caller must own the gobbler they're feeding.
        if (owner != msg.sender) revert OwnerMismatch(owner);

However, this limits composability. Consider adopting semantics similar to isApprovedForAll to allow third parties to feed a Gobbler with owner approval.

DeployBase: Double check wallet address

According to the Art Gobblers whitepaper,

No Pages accrue directly to the team, but one in ten newly created pages do go to a vault to be distributed to artists in the community.

However, in DeployBase.s.sol, the Pages contract is deployed with teamColdWallet as the community argument:

DeployBase.s.sol#L101-L102

        // Deploy pages contract.
        pages = new Pages(mintStart, goo, teamColdWallet, artGobblers, pagesBaseUri);

Double check whether you're passing the correct parameter here.

Pages and GobblerReserve: Prefer preincrement

We have not submitted a separate gas report, but we consider it a sacred duty to point out the following two usages of i++ that could instead use ++i:

Pages.sol#L250-L253

            // Mint the pages to the community reserve while updating lastMintedPageId.
            for (uint256 i = 0; i < numPages; i++) {
                _mint(community, ++lastMintedPageId);
            }

GobblerReserve.sol#L31-L41

    /// @notice Withdraw gobblers from the reserve.
    /// @param to The address to transfer the gobblers to.
    /// @param ids The ids of the gobblers to transfer.
    function withdraw(address to, uint256[] calldata ids) external onlyOwner {
        // This is quite inefficient, but it's okay because this is not a hot path.
        unchecked {
            for (uint256 i = 0; i < ids.length; i++) {
                artGobblers.transferFrom(address(this), to, ids[i]);
            }
        }
    }