A gobbler can be burned for a legendary one before revealed resulting in loss of emission multiple

[code423n4]

Lines of code

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

Vulnerability details

Impact

A gobbler can be burned for a legendary one before its reveal. This will make the user lose his emission multiple, because it won't be added to the sum when it is calculated in the ArtGobblers.mintLegendaryGobbler() function.

In addition, because the new owner of this gobbler will be the zero address, when the gobbler will be revealed the emission multiple, which will be added to the owner of the gobbler, will be added to the zero address, resulting in the zero address accumulating virtual goo balance. This is an unwanted behavior, since this goo balance will be locked and unreachable.

Proof of Concept

I wrote a PoC in foundry to show that it is possible for an innocent user to lose his emission multiple, and for the zero address to gain emission multiple. It uses some of the helpers from the ArtGobblers.t.sol file, so to test it easily you can simply add it to the contract in that file.

function testBurnUnrevealdGobbler() 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();
    assertEq(cost, 69);
    // Reveal the first 68 gobblers
    setRandomnessAndReveal(cost - 1, "seed");

    // Prepare the ids array
    for (uint256 curId = 1; curId <= cost; curId++) {
        ids.push(curId);
    }
    
    // Mint the legendary gobbler
    vm.prank(users[0]);
    uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

    // Legendary is owned by user.
    assertEq(gobblers.ownerOf(mintedLegendaryId), users[0]);

    // Assert that the gobbler hasn't revealed yet
    uint unrevealedGobbler = cost;
    hevm.expectRevert("NOT_MINTED");
    gobblers.ownerOf(unrevealedGobbler);

    // Assert that the emission multiple of the unrevealed gobbler is zero
    assertEq(gobblers.getGobblerEmissionMultiple(unrevealedGobbler), 0);
    // Assert that the emission multiple of the zero address is zero
    assertEq(gobblers.getUserEmissionMultiple(address(0)), 0);

    gobblers.revealGobblers(1);

    // Assert that the emission multiple of the unrevealed gobbler is not zero (i.e. it was revealed)
    assertTrue(gobblers.getGobblerEmissionMultiple(unrevealedGobbler) > 0);
    // Assert that the emission multiple of the zero address is not zero (i.e. the zero address received the virtual goo balance)
    assertTrue(gobblers.getUserEmissionMultiple(address(0)) > 0);
}

Output:

Running 1 test for test/ArtGobblers.t.sol:ArtGobblersTest
[PASS] testBurnUnrevealdGobbler() (gas: 40844231)
Test result: ok. 1 passed; 0 failed; finished in 41.00ms

Tools Used

Manual audit & foundry for the PoC

Recommended Mitigation Steps

Consider allowing the burn of only revealed token, or keep a record of the original owner to add the emission multiple to instead of adding to the current owner.

If the loss of emission multiple is a wanted behavior, the zero address accumulating virtual goo balance will still be a problem, which can be fixed by adding the emission multiple in the revealGobblers function only if the owner is not the zero address.

[Shungy]

Duplicate: #363, #317

[GalloDaSballo]

The warden has shown how, the current system for minting a legendary gobbler will allow burning non-revealed gobblers, with the negative result of getting no multiples from them.

In a situation in which all burned gobblers are non-revealed, the legendary gobbler will have a 0 multiple.

In judging this finding there's 2 key assumptions I'm making:

• Goo is the Yield Token (Similar to farming emissions)
• You can only burn a gobbler you own

This to me means that while the finding is valid and correct, it requires the conditionality of the user burning its own gobblers of its own volition (similar to sending to 0xDead), while purposefully (or recklessly) getting a 0 multiple out of it.

It may be ideal to ensure that the multiple achieved from minting the legendary gobbler is non-zero, however, the conditionality of the "sacrifice", as well as the loss of "yield" and not of the Legendary Gobbler, lead me to believe this is a Medium Severity finding.

[FrankieIsLost]

This is intended behaviour. The value of a legendary gobbler is not only the emission multiple increase, but also the fact that it's a very rare (max 10) token. We don't want to prevent the use of unrevealed gobblers because we believe in giving users the choice. Some may think it's worth it to lose some future emissions in order to ensure they can own a legendary.

[GalloDaSballo]

Will re-evaluate with the context provided by the Sponsor

[GalloDaSballo]

With the added information given by the sponsor I must agree that the loss of Gobbleer, Goo, etc.. can be worth it to get a Legendary Gobbler.

Ultimately the protocol allows people to decide, and some may decide that getting the Legendary is worth loosing the emissions multiple.

Given that perspective, I think the finding is still valid but I will downgrade to Low Severity.

End users should be aware that they can burn their emissionMultiple if they use their Gobblers to mint a legendary, resulting in a possibly below expectation multiple, however they are free to do so at their own volition

L