Party DAO - Emmanuel's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L465 https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L636-L638

Vulnerability details

Impact

Protocol's intended functionality is: If all hosts accept a passed proposal, then veto period for that proposal will be skipped, and Proposal will immediately be "Ready" for execution. Note that the requirement is "ALL HOSTS accept a proposal", but a single host can maliciously increase the numHostsAccepted count, and make it look like all hosts actually accepted the proposal, which will make the proposal ready for execution against the intention of the other hosts. This allows malicious proposals to skip veto period and immediately become ready for execution, which may negatively affect other hosts and the Party in general

Proof of Concept

Looking at LINE 636 of PartyGovernance#accept function, we can see that if the caller is a host, the numHostsAccepted count is incremented. this line prevents the host from voting twice on a proposal.

The problem is, the caller, who is a host can call PartyGovernance#abdicateHost to transfer host status to another address, and then vote again with that address, which would increment the numHostsAccepted count. He can repeat this numHosts times so that this check passes, and _getProposalStatus returns "Ready", which will allow execution of the proposal against the intention of the other hosts

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing the following: Snapshot the Hosts status each time abdicateHost is called, and instead of checking the current host status of msg.sender, the snapshotted host status as at proposalCreationTime should be used.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L479 https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L486

Vulnerability details

Impact

Once a Party adds an authority, it can NEVER remove it even when an upgrade of Authorities is required. Instead, it depends on the authority to abdicate at will.

Proof of Concept

Authorities have "root user privileges", and while they are trusted, there will be scenarios where a Party might want to totally remove an authority and add a new one, (e.g. due to some needed functionality).

Parties have the ability to addAuthority through a proposal, but there is no corresponding function for the party to remove an authority. Instead, Parties rely on the authority to call the abdicateAuthority function to relinquish the authority role.

This means that an authority can be stuck with a Party forever, which is dangerous, given the amount of privileges it has

Tools Used

Manual Review

Recommended Mitigation Steps

Just like the addAuthority function, there should be a removeAuthority function as well which should have the onlySelf modifier

Assessed type

Other