Party DAO - evmboi32's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1122-L1127 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1098-L1100 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L595-L658 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472

Vulnerability details

Impact

A malicious host can skip the executionDelay by himself.

Proof of Concept

• Let's say a party was initialized with 10 hosts.
• If hosts want to skip the executionDelay this needs to hold true

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

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

• So all the hosts would need to vote on the proposal for it to be possible to skip the delay.
• But one host can increase the number of the hosts voted numHostsAccepted as high as he wants.

function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
    ...
    // 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;
    }
    info.values = values;
    emit ProposalAccepted(proposalId, msg.sender, votingPower);

    // Update the proposal status if it has reached the pass threshold.
    if (
        values.passedTime == 0 &&
        _areVotesPassing(
            values.votes,
            values.totalVotingPower,
            _getSharedProposalStorage().governanceValues.passThresholdBps
        )
    ) {
        ...
    }
    return values.votes;
}

• As we can see in the function above the votingPower is not checked to be 0. The getVotingPowerAt can return 0 and this would let anyone vote on the proposal. Since they vote with 0 votes it is not a problem if a user is not a host. But what if the voter is a host?
• Alice is a host and an active member. She votes on a proposal increasing the numHostsAccepted as intended.
• She can then call abdicateHost and transfer host to another EOA that she controls and has a votingPower of >= 0.

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

• From a new EOA she can call accept again and vote with votingPower of 0. But since this EOA is a new host who hasn't voted yet she can vote again and the check passes

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

• She then transfers the host to another EOA she controls and repeats the process until numHostsAccepted is increased to the number of party hosts. This allows her to single-handedly vote X amount of times as different hosts and skip executionDelay without the consent of other hosts.

Tools Used

VS Code

Recommended Mitigation Steps

The easiest way I see this could be solved would be to change the mapping in the following way

struct HostData{
    // is the user a host
    bool isHost;
    // the block number at which he was assigned as a host
    uint256 blockNumber;
}
+ mapping(address => HostData) public hostData;
- mapping(address => bool) public isHost;

Now when setting hosts in the initialize function

for (uint256 i = 0; i < govOpts.hosts.length; ++i) {
    hostData[govOpts.hosts[i]] = HostData({
        isHost: true,
        blockNumber: block.number
    });
}

And require that the user has been assigned as a host before the proposal time when voting

function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
    ...
    // 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]) {
+   if (hostData[msg.sender].isHost && hostData[msg.sender].blockNumber != 0 && hostData[msg.sender].blockNumber <= values.proposedTime - 1 ) {
        ++values.numHostsAccepted;
    }
    ...
}

Then also change the behavior of abdicateHost

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]) {
+       if (hostData[newPartyHost].isHost) {
            revert InvalidNewHostError();
        }
-       isHost[newPartyHost] = true;
+       hostData[newPartyHost].isHost = true;
+       hostData[newPartyHost].blockNumber = block.number;
    } else {
        // Burned the host status
        --numHosts;
    }
-   isHost[msg.sender] = false;
+   hostData[msg.sender].isHost = false;
+   hostData[msg.sender].blockNumber = 0;
    
    emit HostStatusTransferred(msg.sender, newPartyHost);
}

Assessed type

Access Control