Art Gobblers contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L458 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/GobblersERC721.sol#L166

Vulnerability details

Impact

When minting a legendary token (in the ArtGobblers.mintLegendaryGobbler() function), the value of getUserData[msg.sender].gobblersOwned is updated to be getUserData[msg.sender].gobblersOwned - cost + 1. This is done to reduce all the burnt token and add the new legendary gobbler. However, later in this function there's a call to the _mint function, which also adds 1 to the getUserData[msg.sender].gobblersOwned.

This causes the getUserData[msg.sender].gobblersOwned value to decrease by 1 less than it should. This messes up the accounting, as the balance of the user who minted the legendary gobbler will be 1 more than the actual amount of gobblers he owns.

Proof of Concept

I wrote the PoC here as a foundry test, showing that the balance of the legendary gobbler minter decreases by cost - 2 instead of decreasing by cost - 1.

function testWrongBalanceAccounting() 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();
    
    // Prepare the ids array
    for (uint256 curId = 1; curId <= cost; curId++) {
        ids.push(curId);
    }

    // Record the balance before the mint
    uint balanceBefore = gobblers.balanceOf(users[0]);
    
    // Mint the legendary gobbler
    vm.prank(users[0]);
    uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

    // Legendary is owned by user.
    assertEq(gobblers.ownerOf(mintedLegendaryId), users[0]);
    // Record the balance after the mint
    uint balanceAfter = gobblers.balanceOf(users[0]);

    assertEq(balanceAfter, balanceBefore - cost + 2);
    console.log(balanceBefore);
    console.log(balanceAfter);
}

The output:

Running 1 test for test/ArtGobblers.t.sol:ArtGobblersTest
[PASS] testWrongBalanceAccounting() (gas: 39395768)
Logs:
  581
  514

Test result: ok. 1 passed; 0 failed; finished in 28.98ms

Tools Used

Manual audit & foundry for the PoC

Recommended Mitigation Steps

Remove one of the additions, the one in the mintLegendaryGobbler will be easier to remove, however if you want to reduce the gas cost (save some SLOADs) a different solution can be applied.

QA Report

• The current implementation of the ArtGobblers.claimGobbler() function allows each user to claim at most 1 gobbler. It will be better to extend this functionality to allow users to claim an amount of gobblers that will be specified in the merkle tree.
• The mintFromGoo in both of the ArtGobblers and the Pages contracts allows the users to pay goo from his virtual balance or from his actual goo balance. This functionality can be improved by allowing a user to use both if this balances - this will save the users from first adding goo to their virtual balance and then using it, or withdrawing goo from their virtual balance and then using their non virtual balance. This will of course relates to the case where the user doesn't have enough balance to pay in both his virtual and non-virtual balances.

  // From ArtGobblers
  useVirtualBalance
      ? updateUserGooBalance(msg.sender, currentPrice, GooBalanceUpdateType.DECREASE)
      : goo.burnForGobblers(msg.sender, currentPrice);
  
  // From Pages
  useVirtualBalance
      ? artGobblers.burnGooForPages(msg.sender, currentPrice)
      : goo.burnForPages(msg.sender, currentPrice);

• The legendaryGobblerPrice will return a value and won't revert even after all the legendary gobblers will be minted. This can confuse innocent users into thinking another legendary gobbler will be minted/is open for auction. This will happen in 2 unwanted cases - when all the legendary gobblers will be sold, and the value of numMintedFromGoo will be either 6391 or 6392. 6392 is the maximum number of gobblers that can be sold (i.e. MAX_MINTABLE), and when the numMintedFromGoo value will be one of the two I mentioned before, the legendaryGobblerPrice function will return unwanted values.

  // For numMintedFromGoo = 6391, the return value will be 
  FixedPointMathLib.unsafeDivUp(startPrice * 6391, 6392) = 99.98% of the start price
  // For numMintedFromGoo = 6392, the return value will be 
  FixedPointMathLib.unsafeDivUp(startPrice * 6392, 6392) = the start price

• Consider adding a check to the acceptRandomSeed function to verify that the contract is actually waiting for a seed. If somehow the chainlink oracle will call the function multiple times, this can be harmful to the contract.
• The gobble function doesn't allow an operator to feed the gobbler with an NFT. The current implementation reverts if owner != msg.sender, but it shouldn't revert if the msg.sender is an allowed operator, i.e. isApprovedForAll[owner][msg.sender] == true.
• The updateUserGooBalance is an internal function but it doesn't have an underscore in the beginning of its name.
• When a gobbler is burned to buy a legendary gobbler, all the data in the getCopiesOfArtGobbledByGobbler mapping becomes irrelevant. Consider saving the data differently to allow moving it to the legendary gobbler when it is burned.
• Users can spam the logs by providing 0 as the numGobblersEach argument to the ArtGobblers.mintReservedGobblers() function. This can also happen by providing numPages = 0 to the Pages.mintCommunityPages() function
• Missing zero address checks - the constructors don't validate the input, i.e. don't check that the given addresses are not zero.
• The ArtGobblers contract is not an ERC721Receiver. It won't support gobbling of NFTs that their transferFrom function does the onERC721Received callback.
• Consider using the 2-step ownership transfer in order to make sure the new owner is reachable. That way mistakes like wrong address given as the new owner will be prevented.

Gas Optimizations

• Variables like UNREVEALED_URI and BASE_URI, which can't be changed, can be marked as immutable to save gas when accessing them.
• The mapping(uint256 => mapping(address => mapping(uint256 => uint256))) public getCopiesOfArtGobbledByGobbler; mapping can be changed to simplify it. We know the gobbler id will be maximum 10000, so it can fit in a uint96. That means that instead of mapping a gobbler id to an address of an nft, we can group them together and create the new mapping(uint256 => mapping(uint256 => uint256))) public getCopiesOfArtGobbledByGobbler;, where the first uint256 is the gobbler id and the nft contract's address grouped ((gobblerId << 160) | nft).
• Calculate toWadUnsafe(MAX_MINTABLE) in the ArtGobblers contract pre deployment so it will save gas on deployment.
• Cache storage variables to reduce the number of SLOADs
  • Cache legendaryGobblerAuctionData.numSold, getGobblerData[id] in the ArtGobblers.mintLegendaryGobbler
  • Use the cached value in ArtGobblers.legendaryGobblerPrice instead of accessing numMintedFromGoo again (uint256 numMintedSinceStart = numMintedFromGoo - numMintedAtStart; => uint256 numMintedSinceStart = mintedFromGoo - numMintedAtStart;)
  • Cache getGobblerData[swapId].idx and getGobblerData[currentId].idx in the ArtGobblers.revealGobblers function. The optimized code:

    uint idx = getGobblerData[swapId].idx;
    uint64 swapIndex = idx == 0
        ? uint64(swapId) // Hasn't been shuffled before.
        : idx; // Shuffled before.
    
    // Get the owner of the current id.
    address currentIdOwner = getGobblerData[currentId].owner;
    
    // Get the index of the current id.
    idx = getGobblerData[currentId].idx;
    uint64 currentIndex = idx == 0
        ? uint64(currentId) // Hasn't been shuffled before.
        : idx; // Shuffled before.

• Modify the expression legendaryGobblerAuctionData.startPrice = uint120(cost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE / 2 ? LEGENDARY_GOBBLER_INITIAL_START_PRICE : cost * 2); in the ArtGobblers.mintLegendaryGobbler function by:
  1. Changing cost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE / 2 to 2 * cost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE
  2. Changing 2 * cost to cost << 1
  3. Cache the value of cost << 1 instead of calculating it twice The final expression:

  uint doubleCost = cost << 1;
  legendaryGobblerAuctionData.startPrice = uint120(doubleCost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE ? LEGENDARY_GOBBLER_INITIAL_START_PRICE : doubleCost);

• Use shifting instead of multiplying by powers of 2 in the ArtGobblers.mintLegendaryGobbler function (getGobblerData[gobblerId].emissionMultiple = uint32(burnedMultipleTotal * 2) => getGobblerData[gobblerId].emissionMultiple = uint32(burnedMultipleTotal << 1))
• Use prefix incrementation and decrementation when incrementing or decrementing variables by one:
  • line 464 of the ArtGobblers.mintLegendaryGobbler function - legendaryGobblerAuctionData.numSold += 1 => ++legendaryGobblerAuctionData.numSold
  • line 906 - getUserData[from].gobblersOwned -= 1; => --getUserData[from].gobblersOwned;
  • line 913 - getUserData[to].gobblersOwned += 1; => ++getUserData[to].gobblersOwned;
• Create a constant for the last non-legendary gobbler id instead of calculating FIRST_LEGENDARY_GOBBLER_ID - 1 in every iteration of the loop in ArtGobblers.revealGobblers
• Use & (2 ** 64 - 1) instead of % 2 ** 64. (randomSeed := mod(keccak256(0, 32), shl(64, 1)) => randomSeed := and(keccak256(0, 32), 0xffffffffffffffff))
• Optimize this condition using the following steps:
  1. newNumMintedForReserves > (numMintedFromGoo + newNumMintedForReserves) / 5
  2. 5 * newNumMintedForReserves > numMintedFromGoo + newNumMintedForReserves
  3. 4 * newNumMintedForReserves > numMintedFromGoo
  4. (newNumMintedForReserves << 2) > numMintedFromGoo