Art Gobblers contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L422-L442

Vulnerability details

Owner of the gobblers that are used for legendary gobbler purchase can claim all of them back as getApproved, which is present in GobblersERC721 as a part of base ERC721 implementation, isn't removed on the legendary gobbler purchase, only owner is being reset to become zero address.

But as the approved address remains, this address can still transfer a gobbler anywhere, i.e. unburning it. This can be done for every gobbler that took part in the purchase, effectively obtaining the legendary gobbler for free.

Proof of Concept

Suppose Mike gathered array of ids = {id} of non-legendary gobblers big enough to buy a legendary one that is currently being auctioned.

Let's say that it is exact, ids.length == legendaryGobblerPrice(), and Mike's address is the owner of each id.

Mike calls approve(Mike, id) for each id in ids:

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/utils/token/GobblersERC721.sol#L92-L100

    function approve(address spender, uint256 id) external {
        address owner = getGobblerData[id].owner;

        require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "NOT_AUTHORIZED");

        getApproved[id] = spender;

        emit Approval(owner, spender, id);
    }

Mike calls mintLegendaryGobbler(ids):

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L422-L442

        // Overflow should not occur in here, as most math is on emission multiples, which are inherently small.
        unchecked {
            uint256 burnedMultipleTotal; // The legendary's emissionMultiple will be 2x the sum of the gobblers burned.

            /*//////////////////////////////////////////////////////////////
                                    BATCH BURN LOGIC
            //////////////////////////////////////////////////////////////*/

            uint256 id; // Storing outside the loop saves ~7 gas per iteration.

            for (uint256 i = 0; i < cost; ++i) {
                id = gobblerIds[i];

                if (id >= FIRST_LEGENDARY_GOBBLER_ID) revert CannotBurnLegendary(id);

                require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");

                burnedMultipleTotal += getGobblerData[id].emissionMultiple;

                emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id);
            }

Mike's ids now all transferred to zero address.

But as getApproved[id] remained untouched and it is still Mike for every id, now he can call transferFrom(0, Mike, id) successfully for each id:

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L880-L914

    function transferFrom(
        address from,
        address to,
        uint256 id
    ) public override {
        require(from == getGobblerData[id].owner, "WRONG_FROM");

        require(to != address(0), "INVALID_RECIPIENT");

        require(
            msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
            "NOT_AUTHORIZED"
        );

        delete getApproved[id];

        getGobblerData[id].owner = to;

        unchecked {
            uint32 emissionMultiple = getGobblerData[id].emissionMultiple; // Caching saves gas.

            // We update their last balance before updating their emission multiple to avoid
            // penalizing them by retroactively applying their new (lower) emission multiple.
            getUserData[from].lastBalance = uint128(gooBalance(from));
            getUserData[from].lastTimestamp = uint64(block.timestamp);
            getUserData[from].emissionMultiple -= emissionMultiple;
            getUserData[from].gobblersOwned -= 1;

            // We update their last balance before updating their emission multiple to avoid
            // overpaying them by retroactively applying their new (higher) emission multiple.
            getUserData[to].lastBalance = uint128(gooBalance(to));
            getUserData[to].lastTimestamp = uint64(block.timestamp);
            getUserData[to].emissionMultiple += emissionMultiple;
            getUserData[to].gobblersOwned += 1;
        }

Notice that all the getUserData[from] manipulations are unchecked, so the update of getUserData for zero address can get it messed up (emissionMultiple and gobblersOwned can underflow), but the call will not be reverted.

Recommended Mitigation Steps

Consider adding getApproved[id] removal to the gobblerIds burning logic:

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L432-L442

            for (uint256 i = 0; i < cost; ++i) {
                id = gobblerIds[i];

                if (id >= FIRST_LEGENDARY_GOBBLER_ID) revert CannotBurnLegendary(id);

                require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");

                burnedMultipleTotal += getGobblerData[id].emissionMultiple;

+               delete getApproved[id];

                emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id);
            }