`gobble()` can only work for `owner`

[code423n4]

Lines of code

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

Vulnerability details

Impact

Only the owner is allowed to feed the gobbler, but if the NFT is previous approved for transfer, or has an operator set for approvedForAll, the gobble() function will revert. However, if do the transferFrom() first, and call gobble() from the new owner, the call can succeed. Hence the current implementation is not consistent. As a result, the new owner has to bother extra function calls to feed the gobbler.

Proof of Concept

// src/ArtGobblers.sol
    function gobble(
        uint256 gobblerId,
        address nft,
        uint256 id,
        bool isERC1155
    ) external {

        // ...
        if (owner != msg.sender) revert OwnerMismatch(owner);
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

    function gobble(
        uint256 gobblerId,
        address nft,
        uint256 id,
        bool isERC1155
    ) external {

        // ...
        if (!(owner == msg.sender || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id])) revert OwnerMismatch(owner);
    }

[Shungy]

This is informational. It is up to the discretion of devs to take gobbling gobbler's approvals into consideration or not. But the recommendation is wrong as it confuses gobblerId with gobbled NFT ID.

[GalloDaSballo]

Dup of https://github.com/code-423n4/2022-09-artgobblers-findings/issues/298

[GalloDaSballo]

R