Art Gobblers contest - ladboy233'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/ArtGobblers.sol#L693 https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/GobblersERC721.sol#L92

Vulnerability details

Impact

Detailed description of the impact of this finding.

When minting legendary NFT, non-legendary NFTs are burned,

only the owner of the burned nft is set to 0,

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

but burned token approval is not revoked,

burned NFT TokenURI is still accessible after burning.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

we update the token uri for testing purpose in the test case

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/test/ArtGobblers.t.sol#L76

        gobblers = new ArtGobblers(
            keccak256(abi.encodePacked(users[0])),
            block.timestamp,
            goo,
            Pages(pagesAddress),
            address(team),
            address(community),
            randProvider,
            "revealed/",
            "not_revealed/"
        );

We can add the test to ArtGobbler.t.sol

    function testMintLegendaryGobbleBurningCleanUp_POC() 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());

        vm.prank(users[0]);
        gobblers.approve(users[1], 1);

        uint256 cost = gobblers.legendaryGobblerPrice();
        assertEq(cost, 69);
        setRandomnessAndReveal(cost, "seed");
        uint256 emissionMultipleSum;
        //add more ids than necessary
        for (uint256 curId = 1; curId <= cost + 10; curId++) {
            ids.push(curId);
            assertEq(gobblers.ownerOf(curId), users[0]);
            emissionMultipleSum += gobblers.getGobblerEmissionMultiple(curId);
        }

        vm.prank(users[0]);
        gobblers.mintLegendaryGobbler(ids);

        console.log("owner for token id 1 is address(0) after burning");
        hevm.expectRevert("NOT_MINTED");
        console.log(gobblers.ownerOf(1));

        console.log("");
        console.log("---- legendary gobbler is minted --- tokenUI is -----");
        console.log(gobblers.tokenURI(9991));

        console.log("");
        console.log(" --- tokenURI for burned token id 1 still accessible --- ");
        console.log(gobblers.tokenURI(1));

        console.log("");
        console.log(" --- token approval not revoked after token burning ----");
        console.log(" user 1 still got approve for user 0?");
        console.log(gobblers.getApproved(1) == users[1]);

        //check full cost was burned
        for (uint256 curId = 1; curId <= cost; curId++) {
            hevm.expectRevert("NOT_MINTED");
            gobblers.ownerOf(curId);
        }
    
        // check extra tokens were not burned
        for (uint256 curId = cost + 1; curId <= cost + 10; curId++) {
            assertEq(gobblers.ownerOf(curId), users[0]);
        }
    }

we run the test

forge test -vv --match testMintLegendaryGobbleBurningCleanUp_POC

the running is

[PASS] testMintLegendaryGobbleBurningCleanUp_POC() (gas: 41308783)
Logs:
  owner for token id 1 is address(0) after burning
  0x0000000000000000000000000000000000000000

  ---- legendary gobbler is minted --- tokenUI is -----
  revealed/9991

   --- tokenURI for burned token id 1 still accessible ---
  revealed/8876

   --- token approval not revoked after token burning ----
   user 1 still got approve for user 0?
  true

Test result: ok. 1 passed; 0 failed; finished in 55.00ms

note after we mint 69 non-legendary art gobbler nfts,

we approve user 1 to spend nft id 1 on behalf of user 0

        vm.prank(users[0]);
        gobblers.approve(users[1], 1);

then after the burning and minting of the legendary art gobbler, we check

         console.log("");
        console.log(" --- tokenURI for burned token id 1 still accessible --- ");
        console.log(gobblers.tokenURI(1));

        console.log("");
        console.log(" --- token approval not revoked after token burning ----");
        console.log(" user 1 still got approve for user 0?");
        console.log(gobblers.getApproved(1) == users[1]);

but as the test shown above, burned NFT tokenURI still accessible, token approval is not revoked.

Tools Used

foundry

Manual Review

Recommended Mitigation

we recommend the project handle the burning-related logic carefully.

We recommand the project revoke the approval when nft is burned and disable the token metadata access and update

getGobblerData[id]

accordingly.

The compiler version may not support customized error between version 0.8.4

All contracts use the pragma version

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity >=0.8.0;

the customized error are extensively used in the code

    error InvalidProof();
    error AlreadyClaimed();
    error MintStartPending();

(customized error in ArbGobbler.sol)

however, customized errors are not supported until solidity version 0.8.4

https://github.com/ethereum/solidity/releases/tag/v0.8.4

So using the solidity version between 0.8.4 and 0.8.0 can result in compiler error.

We recommand the project use the up-to-date solidity version.

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity >=0.8.4;

Use SafeMint instead of mint when minting NFT.

https://ethereum.stackexchange.com/questions/115280/mint-vs-safemint-which-is-best-for-erc721

If YOU are paying for the minting of tokens, use _mint. The _safeMint might cost you an arbitrary amount of money because of choices made by the recipient of the tokens. This is enough to deter you from considering it.

If THEY are paying for the minting of tokens and you expect buyers to be composing functionality with smart contracts, use _safeMint. There is some marginal benefit of allowing the extra features with smart contracts this allows.

Page NFT do not have owner

the Page NFT do not have owner, which means it cannot be listed on opensea or looksRare as collection

We recommand the project add owner for Page NFT.

ArtGobbler and Page NFT do not implement contractURI method

https://docs.opensea.io/docs/contract-level-metadata

We recommand the project add contractURI method to ERC721 or ERC1155 contract that returns a URL for the storefront-level metadata for your contract.

function Gobble miss the check that if the msg.sender is approved by NFT owner

in the function Gobble

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

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

note the check

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

it only check if the owner is the msg.sender, but does not check if msg.sender is approved by NFT owner like other transfer function is handled,

we recommend the project add the check below

if(owner != msg.sender || !isApprovedForAll(owner, msg.sender) || getApproved(tokenId) != msg.sender) revert OwnerMismatch(owner);

GobblerReserve.sol does not implement onERC721Received Hook

GobblerReserver.sol is minted to receive Art Gobbler ERC721 token.

It is a good practice for a smart contract to implement onERC721Received hook

in safeTransfer is used.

https://docs.openzeppelin.com/contracts/3.x/api/token/erc721

Use multiply symbol instead of left shift operator in function mintReservedGobblers in Art Gobbler

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

we recommand change the line

 uint256 newNumMintedForReserves = numMintedForReserves += (numGobblersEach << 1);

to

 uint256 newNumMintedForReserves = numMintedForReserves += (numGobblersEach * 2);

updated the state variable before event emission

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

instead of

       unchecked {
            // Overflow should be impossible due to supply cap of 10,000.
            emit GobblerClaimed(msg.sender, gobblerId = ++currentNonLegendaryId);
        }

we recommand move ++currentNonLegenaryId before the event mission for better readability, conciseness and consistency.

       unchecked {
            // Overflow should be impossible due to supply cap of 10,000.
            gobblerId = ++currentNonLegendaryId
            emit GobblerClaimed(msg.sender,   gobblerId);
        }

same for the event mission in mintLegendary

function mintLegendaryGobbler

we can change

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

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

to

    delete getGobblerData[id];
    emit Transfer(msg.sender, address(0), id);

Use batch mint for page reserve minting instead of minting in the for loop

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

instead of

for (uint256 i = 0; i < numPages; i++) _mint(community, ++lastMintedPageId);

when minting page,

we recommand the project to use batchMint function here.