code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/script/deploy/DeployBase.s.sol#L61-L105 https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/Pages.sol#L239-L257

Vulnerability details

Impact

When the following run function in the DeployBase contract is called, the Pages contract is deployed by executing pages = new Pages(mintStart, goo, teamColdWallet, artGobblers, pagesBaseUri). Unlike the ArtGobblers contract that uses address(communityReserve) as its community reserve address, the Pages contract's community reserve address is set to the team's cold wallet address, which is incorrect.

https://github.com/code-423n4/2022-09-artgobblers/blob/main/script/deploy/DeployBase.s.sol#L61-L105

    function run() external {
        ...
        communityReserve = new GobblerReserve(ArtGobblers(gobblerAddress), teamColdWallet);
        ...

        // Deploy gobblers contract,
        artGobblers = new ArtGobblers(
            merkleRoot,
            mintStart,
            goo,
            Pages(pageAddress),
            address(teamReserve),
            address(communityReserve),
            randProvider,
            gobblerBaseUri,
            gobblerUnrevealedUri
        );

        // Deploy pages contract.
        pages = new Pages(mintStart, goo, teamColdWallet, artGobblers, pagesBaseUri);

        ...
    }

When the following mintCommunityPages function is called, pages are minted to the community reserve, which, however, is the team's cold wallet. As a result, the minted pages are not distributed to the community, which is unexpected and unfair to the community. Although the DeployRinkeby contract inherits the DeployBase contract currently, it is possible that the deploy script for the mainnet will inherit the DeployBase contract as well since it is the base deploy contract. When this happens, since there is no plan for updating this self-sustaining ecosystem in the future after the upcoming free mint, this issue will be very severe in which the community will always lose the minted pages that should be distributed to it.

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/Pages.sol#L239-L257

    function mintCommunityPages(uint256 numPages) external returns (uint256 lastMintedPageId) {
        unchecked {
            // Optimistically increment numMintedForCommunity, may be reverted below.
            // Overflow in this calculation is possible but numPages would have to
            // be so large that it would cause the loop to run out of gas quickly.
            uint256 newNumMintedForCommunity = numMintedForCommunity += uint128(numPages);

            // Ensure that after this mint pages minted to the community reserve won't comprise more than
            // 10% of the new total page supply. currentId is equivalent to the current total supply of pages.
            if (newNumMintedForCommunity > ((lastMintedPageId = currentId) + numPages) / 10) revert ReserveImbalance();

            // Mint the pages to the community reserve while updating lastMintedPageId.
            for (uint256 i = 0; i < numPages; i++) _mint(community, ++lastMintedPageId);

            currentId = uint128(lastMintedPageId); // Update currentId with the last minted page id.

            emit CommunityPagesMinted(msg.sender, lastMintedPageId, numPages);
        }
    }

Proof of Concept

Please append the following test in test\deploy\DeployRinkeby.t.sol. This test will pass to demonstrate the described scenario.

    function testPagesCommunityReserveAddressIsSetupIncorrectly() public {
        // after running the deploy script,
        // the Pages contract's community reserve address is not set to the address of the deployed community reserve contract
        assertFalse(deployScript.pages().community() == address(deployScript.communityReserve()));

        // however, the Pages contract's community reserve address is set to the team's cold wallet address, which is incorrect
        assertEq(deployScript.pages().community(), deployScript.coldWallet());
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-09-artgobblers/blob/main/script/deploy/DeployBase.s.sol#L102 can be updated to the following code.

        pages = new Pages(mintStart, goo, address(communityReserve), artGobblers, pagesBaseUri);

Shungy commented 2 years ago

Seems legit given deployment script is withing scope.

Shungy commented 1 year ago

I realized the recommended mitigation step is not well-advised. If community reserve address, which is a GobblerReserve.sol contract, is used, then the Pages will be locked because the contract only allows withdrawing only a single NFT defined in its constructor, which would be ArtGobblers NFT. With this information, it is likely that the sponsor used teamColdWallet deliberately here.

There is centralization issue with this design, but from the deployment script, it appears that sponsors want teamColdWallet to control both team and community funds. And the usage of separate GobblerReserve for team and community seems to be for accounting purposes and to burn Goo emissions. Since the Pages only have community reserve, there is no need to have two separate contracts for accounting, and they can be directly stored in teamColdWallet.

I am swapping my "thumbs up" for a "thumbs down" because the finding is likely invalid, if not informational.

GalloDaSballo commented 1 year ago

Personally would not recommend having a cold wallet control any of the contracts nor receive NFTs, especially if they are meant to be shared with somebody else (classic example of risk of rug / risk of hack / opening to man in the middle).

Because the scripts for deployment were in scope, I believe the finding to be valid.

However, we're reviewing a script for deployment, meaning we can't tell what teamColdWallet is, although we can assume it's an EOA.

I would confirm my advice against using an EOA, and recommend using a Gnosis Safe to control the Contracts and to receive Pages.

That said, I think Low Severity is more appropriate.

I do recommend the warden to follow up with the sponsor post-deployment if the setup is still concerning for the community