Finalizing crowdfunds would never work in some instances due to how minTotalContributions & maxTotalContributions are initialized

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Bug case has already being explained here, with protocol's work around for this being that a party's host call can just call finalize() after contributions are more than minTotalContributions

Issue is that this workaround would not always work and could cause some crowdfunds to be stuck in the active state until it expires and ends up being lost.

Proof of Concept

First, take a look at ETHCrowdfundBase.sol#L196-L273

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

        emit Contributed(msg.sender, contributor, amount, delegate);

        // OK to contribute with zero just to update delegate.
        if (amount == 0) return 0;

        // Only allow contributions while the crowdfund is active.
        CrowdfundLifecycle lc = getCrowdfundLifecycle();
        if (lc != CrowdfundLifecycle.Active) {
            revert WrongLifecycleError(lc);
        }
....//omitted for brevity
        //@audit
        // 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 crowdfund 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_);
        }
...//omitted for brevity
    }

In short, as referenced from the full code block and code comments, the current implementation of ETHCrowdfundBase._processContribution() contract inadvertently leads to a scenario where the contract fails to reach the maxTotalContributions due to maybe a high minContribution, specifically this occurs when the remaining balance needed to achieve maxTotalContributions is less than the minContribution.

In such a case, new contributions cannot be made due to a reversion in _processContribution(), effectively causing the crowdfund to stall and can only be finalized if the party host calls on finalize(), but this can only happen if the total contribution is already minTotalContributions or more, but there are no assurances that this would always be the case, because while initializing we can see that minTotalContributions is allowed to be set equal to maxTotalContributions.

The initialization logic can be seen here:

    function _initialize(ETHCrowdfundOptions memory opts) internal {
        // Set the minimum and maximum contribution amounts.
        if (opts.minContribution > opts.maxContribution) {
            revert MinGreaterThanMaxError(opts.minContribution, opts.maxContribution);
        }
        minContribution = opts.minContribution;
        maxContribution = opts.maxContribution;
        // Set the min total contributions.
        //@audit
        if (opts.minTotalContributions > opts.maxTotalContributions) {
            revert MinGreaterThanMaxError(opts.minTotalContributions, opts.maxTotalContributions);
        }
        minTotalContributions = opts.minTotalContributions;
        // Set the max total contributions.
...//omitted for brevity
  }

The above check only prevents minTotalContributions from being greater than maxTotalContributions, but does not consider the scenario where minTotalContributions equals maxTotalContributions, which leads to the deadlock in the crowdfund.

In this logic, if the remaining contribution it takes to reach minTotalContributions in this case, maxTotalContributions too since they are equal is below the minContribution value, then _processContribution() would always revert with the BelowMinimumContributionsError() error, failing to achieve the maxTotalContributions goal, which halts the crowdfunding process when close to its goal.

Tool Used

Manual Review

Recommended Mitigation Steps

As long as minContribution is going to be used, then minTotalContributions != maxTotalContributions should be an invariant that's never broken, i.e., the initialization logic should be replaced with this:

    function _initialize(ETHCrowdfundOptions memory opts) internal {
        // Set the minimum and maximum contribution amounts.
        if (opts.minContribution > opts.maxContribution) {
            revert MinGreaterThanMaxError(opts.minContribution, opts.maxContribution);
        }
        minContribution = opts.minContribution;
        maxContribution = opts.maxContribution;
        // Set the min total contributions.
        //@audit
-        if (opts.minTotalContributions > opts.maxTotalContributions) {
+        if (opts.minTotalContributions >= opts.maxTotalContributions) {
            revert MinGreaterThanMaxError(opts.minTotalContributions, opts.maxTotalContributions);
        }
        minTotalContributions = opts.minTotalContributions;
        // Set the max total contributions.
...//omitted for brevity
  }

Another subtle hint is to ensure that the difference between minTotalContributions & maxTotalContributions is always greater than minContribution, that way we close all the edge cases where this could happen.

Assessed type

Context

[c4-pre-sort]

ydspa marked the issue as duplicate of #552

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

[Bauchibred]

HI @gzeon-c4.

Thanks for judging, but I’d really appreciate you having another review on the validity of this report, unlike other duplicates of #522, this report explains the same issue as #119 that was deemed satisfactory, both talk about how crowdfunding would be stuck since they'd be a DOS on an attempt to finalise them which is due to how the “minTotalContributions & maxTotalContributions are initialized”, i.e this report explains the case of when minTotalContributions == maxTotalContributions is true as explained in the referenced report(#119), where as it’d be fair to note that this report does not talk about the exchangeRateBps being less than 1e4, the core issue is the same, and quoting the Code4rena docs, section on duplication they should be considered duplicates

All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path.

The fact of this being a valid duplicate of #119, can be seen right from the first section of this report, i.e impact section of this report.

Issue is that this workaround would not always work and could cause some crowdfunds to be stalled in the active state until it expires and ends up being lost.

This report also referenced the initialization logic and talked about the bug since they (min/maxContributions) can be set the same, with a code snippet attached.

... because while initializing we can see that minTotalContributions is allowed to be set equal to maxTotalContributions.

The recommended mitigation step of this report is exactly same with the one referenced above(#119), stating that minTotalContributions != maxTotalContributions should be an invariant that’s never broken and even adding a subtle hint.

Note that the warden that submitted #119 also submitted #127 (which is a valid duplicate of #522) cause where as bug cases are very similar, the former has an edge case attached to it and should stand on its own, unlike the latter which imo should be valid but OOS, reason below.

About #522 and it's dups, bug is valid and exists, but I believe they are OOS since that particular bug case is considered a known issue due to the comments in code, and all the batches of reports rightfully duplicated to it(#522) only provide a fix to the known issue, which as far as I know on Code4rena, this does not count to be a valid H/M finding but could be considered QA, also I believe multiple other wardens (myself included) saw the bug case but didn't submit a report cause it's known, which is why I decided to attach a subtle hint in the Recommended Mitigation Steps of this report to help sponsors cover this edge case.

Coming back to this issue & #119, breezing through the pre-sorted batch of #522, I assume only this report and #539 talks about this specific bug case and hint on making minTotalContributions != maxTotalContributions an invariant which should warrant being duplicates of #119.

Thank you for your time.

[0xdeth]

Hey @Bauchibred

Your issue reverts because of the minContribution check and doesn't use the exchangeRate in the attack, while #119 reverts by rounding down the votingPower to 0 to force a revert.

The #119 (comment) states that the attack still works when minContribution = 0, while this issue does not.

This issue is more similar to #453 than #119

Cheers.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

[c4-judge]

gzeon-c4 marked the issue as satisfactory