`gobble()` function can eat more than just ERC1155 or ERC721

[code423n4]

Lines of code

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

Vulnerability details

gobble() function can eat more than just ERC1155 or ERC721

Summary

gobble function for not reverting needs a correct gobblerId which msg.sender is the owner of it, and nft address not to be ArtGobblers contract address.

nft is not being checked to be actually an ERC721 or ERC1155, there is just a boolean to decide which path to take, and it is an user input.

Sending for example a gobblerId which you own, whichever value in nft, isERC1155 = false and whichever id, this can populate getCopiesOfArtGobbledByGobbler without reverting if the address of nft actually includes method called at the end of gobble(), but without actually eating an NFT

POC

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

Github Permalinks

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

Mitigation steps

Add a check for nft address to be ERC1155 or ERC721 and if not revert

[Shungy]

This is a function with no use other than to increment a mapping value. The mapping itself has no use in the contract. I assume it will be used in UI. And in that case it's up to the UI which NFT addresses to show. Obviously, malicious or malformed NFT addresses would not be whitelisted or shown on the UI. If such addresses increment their mapping value then they can do that, there is no harm in it.

[GalloDaSballo]

NC