Party DAO - HChang26's results

Lines of code

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

Vulnerability details

Impact

In the current implementation, hosts in the crowdfunding system possess significant power. This might lead to potential misuse especially hosts can also be a member of the party.

Proof of Concept

Based on Docs:

Hosts are trusted addresses with the ability to unilaterally veto proposals and configure Rage Quit in the Party that is created after the crowdfund is won. While the crowdfund is active, the Host can finalize the crowdfund early if it has reached its minimum crowdfunding goal.

While I understand the Host is a trusted role and should not act maliciously. However, the current implementation (various functions combined) gives hosts too much power considering they can also be a member of the party.

Consider the following scenario: A host, who is also a party member, initiates a proposal using the propose() function. The implementation promptly triggers accept(), marking the host as having voted (info.hasVoted[msg.sender] = true). This, in turn, bars the host from casting further votes on this proposal. Additionally, values.votes increases by the host's votingPower, and values.numHostsAccepted also increments.

        info.hasVoted[msg.sender] = true;

        // Increase the total votes that have been cast on this proposal.
        uint96 votingPower = getVotingPowerAt(msg.sender, values.proposedTime - 1, snapIndex);
        values.votes += votingPower;
        if (isHost[msg.sender]) {
            ++values.numHostsAccepted;
        }
        info.values = values;

The problem arises with the abdicateHost() function. It allows the host to transfer their host status to another address without requiring party approval. Consequently, a host can create multiple accounts and continually accept() proposals that may personally benefit them.

    function abdicateHost(address newPartyHost) external {
        _assertHost();
        // 0 is a special case burn address.
        if (newPartyHost != address(0)) {
            // Cannot transfer host status to an existing host.
            if (isHost[newPartyHost]) {
                revert InvalidNewHostError();
            }
            isHost[newPartyHost] = true;
        } else {
            // Burned the host status
            --numHosts;
        }
        isHost[msg.sender] = false;
        emit HostStatusTransferred(msg.sender, newPartyHost);
    }

This situation leads to a skewed system, as the platform erroneously recognizes that all hosts have accepted the proposal. As a result, the execution delay is bypassed. When a proposal is execute(), other hosts do not the authority to veto() because the ProposalStatus is either inProgress or Complete.

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

Tools Used

Manual Review

Recommended Mitigation Steps

There are 2 ways to prevent this issue:

1. Disallow hosts from being a member of the party.
2. Host should not able to freely transfer host role without approval.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L706 https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L860 https://github.com/code-423n4/2023-10-party/blob/main/contracts/proposals/ProposalExecutionEngine.sol#L146

Vulnerability details

Impact

executeProposal() on ProposalExecutionEngine.sol is not marked as payable. Any proposal that requires using/sending native token will brick.

Proof of Concept

Once a proposal successfully passes governance, it becomes eligible for execution by an active member through the execute() function.

        bool completed = _executeProposal(
            proposalId,
            proposal,
            preciousTokens,
            preciousTokenIds,
            _getProposalFlags(values),
            progressData,
            extraData
        );
        if (!completed) {
            // Proposal did not complete.
            proposalState.values.completedTime = 0;
        }

This triggers the _executeProposal() function after passing essential checks. _executeProposal() will delegateCall ProposalExecutionEngine.sol. However, _executeProposal() encounters a problem when dealing with proposals that involve native tokens since the executeProposal() function in ProposalExecutionEngine.sol lacks the payable modifier. Consequently, executing proposals requiring native tokens results in a failure, causing the proposal to remain in the "inProgress" state and eventually leading to its cancel() by the party.

    function executeProposal(
        ExecuteProposalParams memory params
    ) external onlyDelegateCall returns (bytes memory nextProgressData) {

Tools Used

Manual Review

Recommended Mitigation Steps

    function executeProposal(
        ExecuteProposalParams memory params
-   ) external onlyDelegateCall returns (bytes memory nextProgressData) {
+   ) external payable onlyDelegateCall returns (bytes memory nextProgressData) {
    ...

Assessed type

Payable