code423n4 commented 2 years ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L526-L549

Vulnerability details

Impact

The following propose function is callable by any active member. If an active member becomes malicious, she or he can call propose by providing meaningless inputs for many times. This will create many meaningless proposals and emit many meaningless Proposed events. As a result, the frontend can be spammed and become less efficient. The user experience for other governance members can be degraded as well.

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L526-L549

    function propose(Proposal memory proposal, uint256 latestSnapIndex)
        external
        onlyActiveMember
        onlyDelegateCall
        returns (uint256 proposalId)
    {
        proposalId = ++lastProposalId;
        // Store the time the proposal was created and the proposal hash.
        (
            _proposalStateByProposalId[proposalId].values,
            _proposalStateByProposalId[proposalId].hash
        ) = (
            ProposalStateValues({
                proposedTime: uint40(block.timestamp),
                passedTime: 0,
                executedTime: 0,
                completedTime: 0,
                votes: 0
            }),
            getProposalHash(proposal)
        );
        emit Proposed(proposalId, msg.sender, proposal);
        accept(proposalId, latestSnapIndex);
    }

Proof of Concept

Please append the following test in sol-tests\party\PartyGovernanceUnit.t.sol. This test will pass to demonstrate the described scenario.

    function testCreateManyMeaninglessProposals() public {
        (IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds) =
            _createPreciousTokens(2);
        TestablePartyGovernance gov =
            _createGovernance(100e18, preciousTokens, preciousTokenIds);
        address undelegatedVoter = _randomAddress();

        gov.rawAdjustVotingPower(undelegatedVoter, 1, address(0));

        // This Proposal struct is meaningless.
        PartyGovernance.Proposal memory proposal = PartyGovernance.Proposal({
            maxExecutableTime: 0,
            cancelDelay: 0,
            proposalData: abi.encode("")
        });

        // An active member can use the meaningless Proposal struct to create many meaningless proposals
        for (uint256 i; i < 100; ++i) {
            vm.prank(undelegatedVoter);
            uint256 proposalId = gov.propose(proposal, 0);
            assertEq(proposalId, i + 1);
        }
    }

Tools Used

VSCode

Recommended Mitigation Steps

The number of proposals, which are not executed, canceled, vetoed, or expired, that can be created per user could be capped by a sensible limit.

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 not an issue. Closing as invalid.

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