Party DAO - Shaheen's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L637

Vulnerability details

In the Party Governance system, a proposal cannot be executed until it reaches the READY state. And the READY state can be reached only if one of the following conditions is met:

1. The votes threshold is met, and the execution delay has passed
2. All party members have unanimously accepted the proposal.
3. All hosts in the party have accepted the proposal.

We will focus on the third condition, it is a new addition and currently has a flaw.

When a party members initiates a proposal by calling propose(), two values, numHosts and numHostsAccepted, are recorded. numHosts represents the number of hosts at the time the proposal is created, and numHostsAccepted starts at zero. numHostsAccepted serves as a counter to keep track of how many hosts have accepted the proposal. When a host accept a proposal, this counter gets increased:

        if (isHost[msg.sender]) {
            ++values.numHostsAccepted;
        }

This mechanism ensures that when numHostsAccepted becomes equal to numHosts, the proposal can skip the veto period and become READY for immediate execution, meeting the third condition mentioned above.

There is another functionality: abdicateHost() Which allows current hosts to abdicate their host position. Either to burn it or to transfer it to another address.

    function abdicateHost(address newPartyHost) external {
        _assertHost();
        if (newPartyHost != address(0)) {
            if (isHost[newPartyHost]) {
                revert InvalidNewHostError();
            }
        ///@audit-issue doesn't reset ProposalStateValues.numHostsAccepted for the new host!
        ///@audit-issue 1 host can manipulate other hosts and whole party! -`testGovernance_HostAccept_Abdicate_NewHostAccept_Issue_PoC()`
            isHost[newPartyHost] = true;
        } else {
            --numHosts;
        }
        ///@audit-ok Host's Voting Power not set to zero? - host leaves his position not membership.
        isHost[msg.sender] = false;
        emit HostStatusTransferred(msg.sender, newPartyHost);
    }

The issue lies in this function.

Issue:

abdicateHost() doesn't reset numHostsAccepted for hosts who have already accepted an ongoing proposal. That means if a host accepts a proposal and then he leaves the party's host position during that proposal's voting period, he still will be counted in the numHostsAccepted. This is quite okay if he burns his position but if he transfers his host position to another address then when that new host will vote, he will also going to be added in the numHostsAccepted. This can pose significant issues for other hosts.

Consider this scenario: (Coded PoC demonstrates this)

1. There are 2 Hosts in a party; Bob & Steve (numHosts = 2 & numHostsAccepted = 0)
2. A party member, john creates a proposal. the proposal needs 2 hosts votes to get ready for the execution.
3. Bob accepts the proposal. Now only 1 host votes is needed, "Steve's"
4. Bob decides to abdicate his position and transfer it to Nicholas.
5. Nicholas becomes the new host and he likes the john's proposal, so he accepts it. (numHosts still = 2 but numHostsAccepted = 2).
6. The proposal is now in READY state as numHostsAccepted == numHosts, _hostsAccepted() will return true without Steve's vote.

Worst case scenario:

This issue can be exploited by a malicious host to execute a proposal immediately even when there are 10s Hosts in a party. The malicious Host can simply deploy new contracts to do that. He will first vote, abdicate his role to a new address, vote from their and abdicate to a new address and then vote, abdicate, vote, abdicate, vote, and so on. Since a host can transfer their role to anyone, including non-members of the party, this exploit can be done easily.

Impact

• The _hostsAccepted() can return true without every host's acceptance/vote.
• A malicious Host can execute any proposal immediately even when there are many hosts.

Proof of Concept

Add this test to PartyGovernance.t.sol. For the sake of simplicity, this PoC only takes 2 Hosts into account, "Party & Steve". It shows how without steve's vote the proposal reaches READY/PROPOSAL_FLAG_HOSTS_ACCEPT State:

    function testGovernance_HostAccept_Abdicate_NewHostAccept_Issue_PoC() public {
        (
            Party party,
            IERC721[] memory preciousTokens,
            uint256[] memory preciousTokenIds
        ) = partyAdmin.createParty(
                partyImpl,
                PartyAdmin.PartyCreationMinimalOptions({
                    host1: address(this),
                    host2: address(steve),
                    passThresholdBps: 5000,
                    totalVotingPower: 43,
                    preciousTokenAddress: address(toadz),
                    preciousTokenId: 1,
                    rageQuitTimestamp: 0,
                    feeBps: 0,
                    feeRecipient: payable(0)
                })
            );
        DummySimpleProposalEngineImpl engInstance = DummySimpleProposalEngineImpl(address(party));

        // Mint first governance NFT
        partyAdmin.mintGovNft(party, address(john), 22, address(john));
        assertEq(party.votingPowerByTokenId(1), 22);
        assertEq(party.ownerOf(1), address(john));

        // mint another governance NFT
        partyAdmin.mintGovNft(party, address(danny), 21, address(danny));
        assertEq(party.votingPowerByTokenId(2), 21);
        assertEq(party.ownerOf(2), address(danny));

        // Generate proposal
        PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({
            maxExecutableTime: 9999999999,
            proposalData: abi.encodePacked([0]),
            cancelDelay: uint40(1 days)
        });
        john.makeProposal(party, p1, 2);

        // 2 Hosts: Party & Steve
        // 2 Hosts Accept/Vote needed before skipping veto period
        assertEq(party.numHosts(), 2);

        // Party Accepts proposal
        party.accept(1, 0);

        // Now 1 vote is needed
        _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 22);

        // Party abdicates himeself and passes his host position to nicholas
        party.abdicateHost(address(nicholas));

        assertEq(party.numHosts(), 2);

        // nicholas accepts and the 2 hosts requirement fullfiled - "WITHOUT STEVE VOTE"
        nicholas.vote(party, 1, 0);

        // Now anyone can execute immediately.
        _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Ready, 22);

        // Execute proposal
        john.executeProposal(
            party,
            PartyParticipant.ExecutionOptions({
                proposalId: 1,
                proposal: p1,
                preciousTokens: preciousTokens,
                preciousTokenIds: preciousTokenIds,
                progressData: abi.encodePacked([address(0)])
            })
        );

        // Ensure execution occurred
        _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Complete, 22);
        assertEq(engInstance.getFlagsForProposalId(1), LibProposal.PROPOSAL_FLAG_HOSTS_ACCEPT);
        assertEq(engInstance.getLastExecutedProposalId(), 1);
        assertEq(engInstance.getNumExecutedProposals(), 1);
    }

Tools Used

Shaheeniyat

Recommended Mitigation Steps

1. Do not allow abdicateHost() during an ongoing proposal.
2. If a host has accepted a proposal and then calls "abdicateHost()", decrease the "numHostsAccepted" count for that proposal.

It's advisable to consider the first recommendation, as there are other minor issues with the "abdicateHost()" functionality, especially when a host leaves during an ongoing proposal, which can raise additional issues like Veto period will not be skipped even after all hosts' votes, I have discussed that in my QA Report. Thanks!

Assessed type

Context

Findings Summary:

Low Findings:

L-01: Veto period will not be skipped even after all hosts' votes

The veto period skips when all the hosts accept the proposal. But there is some cases where it will not.

A party member initiates a proposal with an initial count of 10 hosts required for the veto period to be skipped. In order to skip the veto period, the numHostsAccepted must reach 10. However, if a host decides to burn their position by invoking the abdicateHost() function, the actual count of hosts (numHosts) in the party reduces to 9. So, even if all current 9 hosts accept the member's proposal, they will still need to wait for the veto period to elapse because the current count of hosts (numHostsAccepted = 9) does not match the required count of accepted hosts (numHosts = 10).

Mitigation:

1. Do not allow abdicateHost() during an ongoing proposal.
2. Decrease the numHosts for the on-going proposals if a host burns his position

L-02: Veto period will not be skipped even after all hosts' votes if the new Host had already accepted the proposal

A party member initiates a proposal with an initial requirement of 10 hosts for the veto period to be bypassed. To achieve this, the numHostsAccepted must reach 10. However, during the voting process, if a host opts to transfer their position to a new host using the abdicateHost(newHost) function, and if the new host had already accepted the proposal before receiving the host position, they will be unable to vote again. Consequently, they will not be included in the numHostsAccepted count. As a result, even if every remaining host votes, the proposal will still fail to meet the criteria of numHostsAccepted (9) == numHosts (10).

Again all hosts voted but the veto period not skipped.

Mitigation:

1. Do not allow abdicateHost() during an ongoing proposal xD
2. create a new function for newHosts to register themselves in the numHostsAccepted when they have already voted for the proposal.

Note: The L-01 & L-02 the edge-cases of the high-severity issue I have submitted and discussed in detail. For additional information, please refer to the high issue titled: _hostsAccepted() can return true even without all hosts' votes, which can be exploited by a malicious host to execute any proposal immediately.

L-03: Base Mainnet do not support 0.8.20, 0.8.19 recommended

The protocol contracts use 0.8.20 solidity pragma version which is not supported yet on bedrock chains as Base Docs state: "Aside from the above, Base Mainnet and Testnet do not yet support the new PUSH0 opcode introduced in Shanghai, which is the default target for the Solidity compiler if you use version 0.8.20 or later. We recommend using 0.8.19 or lower until this is upgraded"

Migitation

     // SPDX-License-Identifier: GPL-3.0
-    pragma solidity 0.8.20;
+    pragma solidity 0.8.19;

Request for the judge: In a previous C4 contest, this was accepted as a valid medium finding, to me it doesn't look medium severity that's why I'm submitting it as a Low severity. But if this gets accepted as a medium again then pls judge sahab bump this to medium as well. Thanks!

L-04: decreaseTotalVotingPower() can decease the totalVotingPower more than the already mintedVotingPower

decreaseTotalVotingPower() doesn't make sure that it doesn't decrease the totalVotingPower more than the already mintedVotingPower. Which can result in overflows/underflow errors exceptions for the users

Make sure to add this check:

    function decreaseTotalVotingPower(uint96 votingPower) external {
       _assertAuthority();

       _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
       require(_getSharedProposalStorage().governanceValues.totalVotingPower < mintedVotingPower, "ERR")
    }

L-05: Important users' input authentications missing

This issue contains all the important missing user inputs checks:

1. Equlity check for the length of values array & tokenIds array missing

The dev comment in BatchContributeArgs states that:

// The length of this (values) array must be equal to the length of tokenIds.

But this check is missing in the batchContribute()

function batchContribute(
        BatchContributeArgs calldata args
    ) external payable onlyDelegateCall returns (uint96[] memory votingPowers) {
        ///@audit-issue args.values == args.tokenIds.length not checked!
        uint256 numContributions = args.tokenIds.length;
        votingPowers = new uint96[](numContributions);
        uint256 ethAvailable = msg.value;
        .....
}

Make sure to add it.

2. Zero check missing for voteDuration & executionDelay

voteDuration & executionDelay are important so a shark cannot propose, accept & execute directly. If it accidentaly gets set to zero it can cause lot of problems so consider adding Zero checks