code423n4 commented 2 years ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyFactory.sol#L26-L53

Vulnerability details

Impact

The following createParty function is external and can be callable by anyone. A malicious actor is able to call createParty by providing meaningless inputs for many times. This will create many meaningless parties and emit many meaningless PartyCreated events. As a result, the frontend can be spammed and become less efficient.

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyFactory.sol#L26-L53

    function createParty(
        address authority,
        Party.PartyOptions memory opts,
        IERC721[] memory preciousTokens,
        uint256[] memory preciousTokenIds
    )
        external
        returns (Party party)
    {
        // Ensure a valid authority is set to mint governance NFTs.
        if (authority == address(0)) {
            revert InvalidAuthorityError(authority);
        }
        // Deploy a new proxified `Party` instance.
        Party.PartyInitData memory initData = Party.PartyInitData({
            options: opts,
            preciousTokens: preciousTokens,
            preciousTokenIds: preciousTokenIds,
            mintAuthority: authority
        });
        party = Party(payable(
            new Proxy(
                GLOBALS.getImplementation(LibGlobals.GLOBAL_PARTY_IMPL),
                abi.encodeCall(Party.initialize, (initData))
            )
        ));
        emit PartyCreated(party, msg.sender);
    }

Proof of Concept

Please add the following event and mapping and append the following test in sol-tests\party\PartyFactory.t.sol. This test will pass to demonstrate the described scenario.

    event PartyCreated(Party party, address creator);
    mapping(address => bool) private meaninglessParties;

    function testCreateManyMeaninglessParties() external {
        // The followings are meaningless party options.
        address someone = _randomAddress();

        IERC721[] memory meaninglessTokens = new IERC721[](1);
        uint256[] memory meaninglessTokenIds = new uint256[](1);
        meaninglessTokens[0] = IERC721(address(0));
        meaninglessTokenIds[0] = 0;

        Party.PartyOptions memory opts = Party.PartyOptions({
            governance: PartyGovernance.GovernanceOpts({
                hosts: _toAddressArray(address(0)),
                voteDuration: 0,
                executionDelay: 0,
                passThresholdBps: 0,
                totalVotingPower: 0,
                feeBps: 0,
                feeRecipient: payable(address(0))
            }),
            name: "abc",
            symbol: "abc"
        });

        // Anyone can use the meaningless options to create many meaningless parties that result in different party addresses.
        for (uint256 i; i < 100; ++i) {
            vm.prank(someone);
            Party party = factory.createParty(
                someone,
                opts,
                meaninglessTokens,
                meaninglessTokenIds
            );
            assertEq(meaninglessParties[address(party)], false);
            meaninglessParties[address(party)] = true;
        }
    }

Tools Used

VSCode

Recommended Mitigation Steps

The createParty function can be updated to be only callable by the Crowdfund contract, instead of being callable by anyone.

merklejerk commented 2 years ago

Not a protocol issue. We can always add additional filtering criteria on the FE.

HardlyDifficult commented 2 years ago

Agree - closing as invalid.

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