Party DAO - bart1e's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1097-L1100 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1126 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L465

Vulnerability details

Short description

If some host first votes on a proposal, then abdicates, but gives a host role to a different address and both new host and all other hosts accept the proposal, its execution delay will not be skipped.

Moreover, any host can inflate numHostsAccepted as much as he wants, causing _hostsAccepted to return true (and hence, execution delay to be skipped).

Detailed description

There is the new functionality in the Party protocol added to skip proposal's execution delay when all hosts accept the proposal.

First, when a proposal is created, the current number of hosts is stored (as numHosts) in the struct representing the proposal:

/ 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,
                totalVotingPower: _getSharedProposalStorage().governanceValues.totalVotingPower,
                numHosts: numHosts,
                numHostsAccepted: 0
            }),
            getProposalHash(proposal)

Then if any host accepts the proposal, its numHostsAccepted is incremented:

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

While the number of hosts cannot increase for a party, it is still possible for numHostsAccepted to be strictly greater than numHosts for a proposal.

In order to see that, consider the following scenario:

1. There are three users: Alice, Bob and Charlie, where both Alice and Bob are hosts.
2. A proposal is created and Alice accepts it.
3. However, for some reason, Alice decides to abdicate and makes Charlie the new host.
4. Bob and Charlie accept the proposal and the proposal reaches enough votes to be in the Passed state.
5. The proposal is in the Passed state, but since all hosts have accepted it, it should be in the Ready state, so that its execution delay is skipped. However, it is not the case, because, when the proposal state is being checked, the following code will execute:

            if (_hostsAccepted(pv.numHosts, pv.numHostsAccepted)) {
                return ProposalStatus.Ready;
            }
            // Passed.
            return ProposalStatus.Passed;

As we see, _hostsAccepted will be called:

    function _hostsAccepted(
        uint8 snapshotNumHosts,
        uint8 numHostsAccepted
    ) private pure returns (bool) {
        return snapshotNumHosts > 0 && snapshotNumHosts == numHostsAccepted;
    }

But it will return false, since numHostsAccepted == 3 and snapshotNumHosts == 2. So the proposal will not be in the Ready state because more hosts accepted it than snapshotNumHosts. Paradoxically, if one host less accepted the proposal, it would have been in the Ready state.

abdicateHost can also be exploited by any host so that he can cause numHostsAccepted == snapshotNumHosts. He can achieve this by calling abdicateHost with any other account that he controls, calling accept and calling abdicateHost once again - he may repeat this procedure as many times as he wants, until numHostsAccepted == snapshotNumHosts.

Impact

Proposals that could have been executed immediately when all hosts accept them will have to wait for the execution delay period to end, which is a form of denial of service.

Of course, hosts can still veto that proposal, create the new one, but they would have to wait for each other to accept it once again and for users to vote for it so that it passes the votes threshold.

In either scenario, it won't be possible to execute the proposal straight away, as intended by the protocol, so the additional waiting is needed.

Moreover, any single host can cause any proposal that has enough votes to be executed immediately by inflating numHostsAccepted (possible to do even in one transaction), although it should require all hosts to call accept.

Proof of Concept

Please place the following test in the PartyGovernance.t.sol file and run it:

function testStatusNotReadyWhenAllHostsAccept() 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)
                })
            );

        // 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);

        // the first host (steve) accepts proposal
        steve.vote(party, 1, 0);

        // steve abdicates and nicholas is the next host
        vm.prank(address(steve));
        party.abdicateHost(address(nicholas));
        
        // address(this) accepts proposal as well
        party.accept(1, 0);

        // finally, nicholas, as the third host accepts the proposal
        vm.prank(address(nicholas));
        party.accept(1, 0);

        // now `snapshotNumHosts == 2 < numHostsAccepted == 3`,
        // so the proposal is in the Passed state although all hosts accepted it and it should be Ready
        _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 22);

        // john will fail to execute proposal, although he should be able to execute it immediately
        vm.expectRevert();
        john.executeProposal(
            party,
            PartyParticipant.ExecutionOptions({
                proposalId: 1,
                proposal: p1,
                preciousTokens: preciousTokens,
                preciousTokenIds: preciousTokenIds,
                progressData: abi.encodePacked([address(0)])
            })
        );
    }

Tools Used

VS Code

Recommended Mitigation Steps

Change the return value of PartyGovernance::_hostsAccepted to: snapshotNumHosts > 0 && snapshotNumHosts <= numHostsAccepted.

In order to solve the second issue (exploiting abdicateHost), it may be necessary to introduce a mapping that will, for each host, return a timestamp when he became a host. If that timestamp is greater or equal to the voting proposedTime, then do not increment values.numHostsAccepted in accept - in fact this change will fix the first issue as well.

Assessed type

Other