Art Gobblers contest - m9800's results

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L411 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/GobblersERC721.sol#L75 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L889 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L441

Vulnerability details

Impact

A User can mint a legendary Gobbler for free.

Proof of Concept

The problem lies in GobblersERC721 approval storage. A user can get approval for a specific id. This approval is granted by the owner of the id or by an "all-approved" user of the owner.

  mapping(uint256 => address) public getApproved;

This approval is not deleted when a Gobbler is burned to pay for a legendary Gobbler. Instead, the ownership is set to address(0). The person having approval for an id has the power to transfer it.

 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);
            }

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"
        );

So the process to obtain a legendary gobbler for free would be :

1. Attacker gives himself approval for all his gobblers
2. Mint a legendary gobbler
3. call transferFrom() and transfer the gobblers to him again.

Recommended Mitigation Steps

When a gobbler is burned to mint a legendary Gobbler, delete the approval of the gobbler's id. (like it's done in transferFrom())

delete getApproved[id];

Lines of code

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

Vulnerability details

Impact

A gobbler con eat any contract implementing safeTransferFrom() or transferFrom() but not compliant with ERC721 or ERC1155.

Proof of Concept

gobble() is not performing any checks.

function gobble(
        uint256 gobblerId,
        address nft,
        uint256 id,
        bool isERC1155
    ) external {
        // Get the owner of the gobbler to feed.
        address owner = getGobblerData[gobblerId].owner;

        // The caller must own the gobbler they're feeding.
        if (owner != msg.sender) revert OwnerMismatch(owner);

        // Gobblers have taken a vow not to eat other gobblers.
        if (nft == address(this)) revert Cannibalism();

        unchecked {
            // Increment the # of copies gobbled by the gobbler. Unchecked is
            // safe, as an NFT can't have more than type(uint256).max copies.
            ++getCopiesOfArtGobbledByGobbler[gobblerId][nft][id];
        }

        emit ArtGobbled(msg.sender, gobblerId, nft, id);

        isERC1155
            ? ERC1155(nft).safeTransferFrom(msg.sender, address(this), id, 1, "")
            : ERC721(nft).transferFrom(msg.sender, address(this), id);
    }

Recommended Mitigation Steps

check if the input nft contract is ERC721 or ERC1155 compliant.