Party DAO - pontifex's results

Lines of code

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

Vulnerability details

Impact

An owner of a host account can bypass the executionDelay and immediately execute the Passed proposal. In case the owner has sufficient voting power it is even possible to accept and execute any proposal which does not have special restrictions.

Proof of Concept

Only proposals with statuses Ready or InProgress can be executed via the execute function of the PartyGovernance contract.

        // The proposal must be executable or have already been executed but still
        // has more steps to go.
        if (status != ProposalStatus.Ready && status != ProposalStatus.InProgress) {
            revert BadProposalStatusError(status);
        }

After the proposal has received the Passed status, executionDelay time must pass to be executed. This can be skipped if the numHosts of hosts voted through accept.

            if (pv.passedTime + gv.executionDelay <= t) {
                return ProposalStatus.Ready;
            }
            // If unanimous, we skip the execution delay.
            if (_isUnanimousVotes(pv.votes, pv.totalVotingPower)) {
                return ProposalStatus.Ready;
            }
            // If all hosts voted, skip execution delay
            if (_hostsAccepted(pv.numHosts, pv.numHostsAccepted)) {
                return ProposalStatus.Ready;
            }

The accept function has a msg.sender check that does not allow the same address to vote twice.

        // Cannot vote twice.
        if (info.hasVoted[msg.sender]) {
            revert AlreadyVotedError(msg.sender);
        }
        // Mark the caller as having voted.
        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;
        }

But an owner of a host can vote and then transfer the role of the host to another controlled address via the abdicateHost and vote as a host again. Repeat this until numHosts is reached. Thus executionDelay will be skipped.

    /// @notice Transfer party host status to another.
    /// @param newPartyHost The address of the new host.
    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);
    }

Moreover, if the owner of a host has sufficient voting power, this allows the immediate execution of any proposal that is not protected by additional procedures.

Tools Used

Manual review

Recommended Mitigation Steps

Consider tightening the transfer of the host role or adding a transfer timestamp for the role to be taken into account when voting. The solution might look like this:

-206    mapping(address => bool) public isHost;
+206    mapping(address => uint40) public isHost;

-217        if (!isHost[msg.sender]) {
+217        if (isHost[msg.sender] == 0) {

-244        if (msg.sender != partyDao && !isHost[msg.sender]) {
+244        if (msg.sender != partyDao && isHost[msg.sender] == 0) {    

-306            isHost[govOpts.hosts[i]] = true;
+306            isHost[govOpts.hosts[i]] = uint40(block.timestamp);

-462            if (isHost[newPartyHost]) {
+462            if (isHost[newPartyHost] != 0) {

-465            isHost[newPartyHost] = true;
+465            isHost[newPartyHost] = uint40(block.timestamp);

-470        isHost[msg.sender] = false;
+470        isHost[msg.sender] = 0;

-636        if (isHost[msg.sender]) {
+636        if (isHost[msg.sender] != 0 && isHost[msg.sender] <= values.proposedTime - 1) {    
637            ++values.numHostsAccepted;
638        }                

Assessed type

Governance