Contributions can be smaller than minContribution and may receive no voting power

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-party/blob/440aafacb0f15d037594cebc85fd471729bcb6d9/contracts/crowdfund/ETHCrowdfundBase.sol#L169-L234

Vulnerability details

Impact

Valid contribution is awarded no voting power

Proof of Concept

ETHCrowdfundBase.sol#L195-L219

    uint96 minContribution_ = minContribution;
    if (amount < minContribution_) {
        revert BelowMinimumContributionsError(amount, minContribution_);
    }
    uint96 maxContribution_ = maxContribution;
    if (amount > maxContribution_) {
        revert AboveMaximumContributionsError(amount, maxContribution_);
    }

    uint96 newTotalContributions = totalContributions + amount;
    uint96 maxTotalContributions_ = maxTotalContributions;
    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.
        uint96 refundAmount = newTotalContributions - maxTotalContributions;
        if (refundAmount > 0) {
            amount -= refundAmount; <- @audit-issue amount is reduced after min check
            payable(msg.sender).transferEth(refundAmount);
        }

When processing a contribution, if the amount contributed would push the crowdfund over the max then it is reduced. This is problematic because this reduction occurs AFTER it checks the amount against the minimum contribution. The result is that these contributions can end up being less than the specified minimum.

Although an edge case, if amount is smaller than exchangeRateBps as it could result in the user receiving no voting power at all for their contribution.

Tools Used

Manual Review

Recommended Mitigation Steps

Enforce minContribution after reductions to amount

[0xble]

This was done intentionally.minContribution may be bypassed for the last contributor when the remaining contribution is less than the min otherwise the party may never be able to reach maxTotalContributions.

Although an edge case, if amount is smaller than exchangeRateBps as it could result in the user receiving no voting power at all for their contribution.

For this though we can add a check to ensure that votingPower != 0.

[c4-sponsor]

0xble marked the issue as sponsor acknowledged

[c4-judge]

0xean marked the issue as satisfactory

[0xble]

Thinking about this one more, it may be worth enforcing the min/max contribution checks after this happens to not allow for this case where contribution < minContribution. The way it currently is creates a potentially bad scenario, as pointed out, where the last contributor may receive no voting power or not be able to claim their party card if this was a ReraiseETHCrowdfund.

The motivation for allowing minContribution to be bypassed for the last contributor when the remaining contribution is less than minContribution was to allow parties to reach maxTotalContributions in a case where they otherwise wouldn't be able to because the contribution to reach it would be below minContribution. However, in this scenario the crowdfund can still be won if either the crowdfund expires above minTotalContribution or a host finalizes at any point after minTotalContribution has been reached. So given the party has recourse if this were to happen, it makes more sense not to allow for this edgecase where contribution < minContribution.

Will mitigate by enforcing minContribution after reductions to the contribution amount.

[c4-sponsor]

0xble marked the issue as sponsor confirmed