code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/GobblerReserve.sol#L34-L41 https://github.com/transmissions11/solmate/blob/main/src/auth/Owned.sol#L19-L23 https://github.com/code-423n4/2022-09-artgobblers/blob/main/script/deploy/DeployBase.s.sol#L61-L105

Vulnerability details

Impact

The following withdraw function can be called by the owner of the team or community reserve contract to withdraw gobblers from the reserve to a receiving address of choice as allowed by the onlyOwner modifier below. As the run function below in the DeployBase contract shows, the team's cold wallet is used as this owner after deployment. If the owner of the team's cold wallet becomes compromised or malicious in the future, such owner can call withdraw to withdraw specified gobblers from the respective reserve to a specified receiving address, which takes effect immediately. As a result, the team or community reserve loses these reserved gobblers in an unauthorized way. Although this risk might be somewhat acceptable to the team since the team has the responsibility to protect its own cold wallet, it is unfair to the community in which it loses its reserved gobblers because it is affected by another party's decision that is not under its control.

https://github.com/code-423n4/2022-09-artgobblers/blob/main/src/utils/GobblerReserve.sol#L34-L41

    function withdraw(address to, uint256[] calldata ids) external onlyOwner {
        // This is quite inefficient, but it's okay because this is not a hot path.
        unchecked {
            for (uint256 i = 0; i < ids.length; i++) {
                artGobblers.transferFrom(address(this), to, ids[i]);
            }
        }
    }

https://github.com/transmissions11/solmate/blob/main/src/auth/Owned.sol#L19-L23

    modifier onlyOwner() virtual {
        require(msg.sender == owner, "UNAUTHORIZED");

        _;
    }

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

    function run() external {
        ...

        // Deploy team and community reserves, owned by cold wallet.
        teamReserve = new GobblerReserve(ArtGobblers(gobblerAddress), teamColdWallet);
        communityReserve = new GobblerReserve(ArtGobblers(gobblerAddress), teamColdWallet);
        ...
    }

Proof of Concept

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

    function testWithdrawFromReservesByCompromisedOrMaliciousOwner() public {
        mintGobblerToAddress(users[0], 9);

        gobblers.mintReservedGobblers(1);

        assertEq(gobblers.ownerOf(10), address(team));
        assertEq(gobblers.ownerOf(11), address(community));

        uint256[] memory idsToWithdraw = new uint256[](1);

        // simulate receiver address for the exploit
        address exploitReceiver = address(1);

        // simulate the situation when the owner of the team and community reserve contracts becomes compromised or malicious
        // for this test suite, this contract is the owner of the team and community reserve contracts
        vm.startPrank(address(this));

        // the compromised or malicious owner calls withdraw, which takes effect immediately
        idsToWithdraw[0] = 10;
        team.withdraw(exploitReceiver, idsToWithdraw);

        idsToWithdraw[0] = 11;
        community.withdraw(exploitReceiver, idsToWithdraw);

        vm.stopPrank();

        // afterwards, the reserved gobblers have been withdrawn from the team and community reserves to exploitReceiver
        assertEq(gobblers.ownerOf(10), exploitReceiver);
        assertEq(gobblers.ownerOf(11), exploitReceiver);
    }

Tools Used

VSCode

Recommended Mitigation Steps

At least for running the withdraw function for the community reserve, a governance mechanism could be set up for the community to vote and decide on whether withdraw can be called or not, instead of allowing the owner of the community reserve contract to directly call withdraw.

Shungy commented 2 years ago

Duplicate: https://github.com/code-423n4/2022-09-artgobblers-findings/issues/431

We have to assume the owner is the community or the team itself (e.g.: multisig or governance contract). Therefore I think these findings are invalid.

GalloDaSballo commented 2 years ago

The presence or absence of a multisig are not in-scope because the Sponsor could lie / cannot be falsified https://github.com/code-423n4/org/issues/7 https://github.com/code-423n4/org/issues/11

The contract allows the "owner" to transfer to any address, it works as intended and no abnormal behaviour was shown.

code-423n4 / 2022-09-artgobblers-findings