Sensitive Bug, disclosed via Code4rena HelpDesk

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/IPartyFactory.sol#L2

Vulnerability details

Filler, LineNumber is also random

Assessed type

Other

Bug Description

The current crowdfund implementation enforces restrictions on the minimum and maximum total contributions it can receive, as well as the minimum and maximum size of an individual contribution. However, a vulnerability exists because the crowdfund neglects to check if the difference between minTotalContributions and maxTotalContributions is smaller than minContribution.

Due to this a malicious user can push the totalContributions into a state where the deposited funds are below the minTotalContributions but if another user would donate the minContribution it would be pushed above the maxTotalContributions. In this case all users would have to wait for the crowdfund to expire. As the duration can be set up to type(uint40).max which is 34,842 years, in the worst case all the users funds will be effectively lost.

Impact

The impact of this issue varies based on the duration set for the crowdfund. In the best-case scenario, users' funds are temporarily locked up until the crowdfund concludes, typically within a timeframe such as 7 days (as used in the testcases). However, users can specify the duration, and an arbitrarily high value could lead to a prolonged freeze, possibly rendering the funds unrecoverable within their lifetime.

If maxTotalContributions - minTotalContributions is small a malicious user would just need to sacrifice a small amount to freeze an arbitrary size amount of other user funds. If the crowdfund does not run for too long, the user could do this just to DOS other users as he will anyways be able to reclaim his funds using refund(). If the expiry time is very high, the malicious user would need to sacrifice his funds too, but if he would be able to freeze 1000s of ETH for the cost of a single ETH, it can still be a valid attack path for a malicious actor.

So in summary we have 2 different impact levels here. If the expiry time is within a reasonable timeframe a attacker is able to DOS a crowdfund for the predefined timeframe, which will most likely be above 15 minutes. This could for example be for a NFT he also wants to bid on. If the expiration timeframe will be large, a user will be able to freeze other users funds "forever", by sacrificing some of his funds.

The only way the funds could be rescued is by using the emergencyExecute() functionality. If this functionality is disable the user funds will be stuck until the end of the expiry period.

Proof of Concept

The provided test case exemplifies the described problem. Users contribute to a crowdfund, and just before reaching the full amount, a malicious user deposits the exact amount to prevent the crowdfund from closing. Now the other users need to wait until the end of the expiry to be able to reclaim their funds.

    function test_malciousUserCanDosCrowdfund() public {
        // --------- SETUP ------------
        InitialETHCrowdfund crowdfund = _createCrowdfund(
            CreateCrowdfundArgs({
                initialContribution: 0,
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: 4 ether,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false,
                minTotalContributions: 98 ether,
                maxTotalContributions: 100 ether,
                duration: 100 days,
                exchangeRateBps: 1e4,
                fundingSplitBps: 0,
                fundingSplitRecipient: payable(address(0)),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            })
        );
        Party party = crowdfund.party();

        assertTrue(crowdfund.getCrowdfundLifecycle() == ETHCrowdfundBase.CrowdfundLifecycle.Active);

        //This address simulates all users of the party
        address member = _randomAddress();
        vm.deal(member, 94 ether);

        //Thsi address simulates a malicious user
        address maliciousUser = _randomAddress();
        vm.deal(maliciousUser, 7 ether);

        //--------- SETUP END ------------

        // Over a certain timeframe all the users contribute to the crowdfund
        vm.prank(member);
        crowdfund.contribute{ value: 90 ether }(member, "");

        //Now the malicious user deposits exactly enough to push the crowdfund in the state where it cna never get finalized
        vm.prank(maliciousUser);
        crowdfund.contribute{ value: 7 ether }(maliciousUser, "");

        //Another normal user will try to deposit the min deposit
        vm.prank(member);
        vm.expectRevert();
        crowdfund.contribute{ value: 4 ether }(member, "");

        //The crowdfund now can not be finalized, not even by the admin. All funds will be stuck until the crowdfund expires.
    }

The following testcase can be added to the InitialETHCrowdfund.t.sol file and run using forge test -vvvv --match-test "test_malciousUserCanDosCrowdfund"

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, the crowdfund initialization process should include a check to ensure that maxTotalContributions - minTotalContributions is greater than the minContribution. This can be implemented by incorporating the following check into the _initialize() function of ETHCrowdfundBase:

if (opts.maxTotalContributions - opts.minTotalContributions < opts.minContribution) {
	revert MinMaxDifferenceTooSmall(opts.minTotalContributions, opts.maxTotalContributions);
}

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[thebrittfactor]

For transparency, the sensitive disclosure contents were not included in the original submission. After sponsor review and approval, the original content has been added.

[c4-sponsor]

0xble (sponsor) confirmed

[0xble]

Would like to note this setup is not currently possible in production due to the way we allow crowdfunds to be configured for ETH raises in our UI at party.app. This is because either minTotalContributions == maxTotalContributions or maxTotalContributions == type(uint96).max depending on the option the Party creator selects when they choose "Allow Flexible Contributions" in our "Start a Party" page.

[c4-pre-sort]

ydspa marked the issue as sufficient quality report

[ydspa]

@0xble & @judge if this would be eventually accepted, please mark these batch #552 as dups of this one.

my initial opinion is OOS, I left a comment earlier at #552 (comment)

[0xble]

Posting this here too for reference:

Upon reflection, while it is similar to the issue pointed out in the comment, the finding does add nuance beyond what was acknowledged in our code comment and points out a scenerio that goes beyond that mentioned. We'd like to consider it valid. It also helps that the mitigation is strong, we'd like to implement it

In our comment we mention "In this scenario users will have to wait until the crowdfund expires or a host finalizes", but in this finding's scenario it would not be possible for a host to finalize.

Another, probably more significant, factor behind us changing our minds to consider this valid and "sponsor confirmed" is that the mitigation proposed cleanly prevents the scenario from happening.

Perhaps we are being too lenient here to consider it valid. I do not entirely disagree with #552 (comment). I'll just give the reasoning behind why we chose to confirm the issue, but would like to leave it to @judge to ultimately decide as I lack context on the standard C4 usually applies to validate finding.

[gzeon-c4]

This is clearly out-of-scope, the fact that the proposed mitigation is good should not affect the validity of this issue for fairness.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #552

[c4-judge]

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

[J4X-98]

@gzeon-c4 Thanks for taking a look at the issue. I'd like to emphasize that, in my perspective, this issue should not be considered out of scope based on the following arguments.

The rationale for categorizing this issue as out of scope is primarily linked to the comment in Line 247-254. However, it's crucial to note that the comment discusses a different scenario and severity. Specifically, the comment states, "In this scenario users will have to wait until the crowdfund expires or a host finalizes after minTotalContribution has been reached by calling finalize()". It's important to highlight that the host finalizing the crowdfund is not feasible in the context showcased and validated by the POC. In the case I've outlined, no host has the capability to prematurely close the fund and rescue user funds. This leads to a different threat than described by the comment, which incorrectly gives users the security of their funds being rescued by a host in the case of a crowdfund getting stuck.

Furthermore, the comment illustrates a situation where minTotalContributions can still be reached by depositing, while maxTotalContributions cannot be achieved. This state can be described as an invariant: minTotalContributions <= potentialDeposits < maxTotalContributions. In the case I've presented, neither of these thresholds can be reached. The invariant for my scenario reads as follows: potentialDeposits < minTotalContributions <= maxTotalContributions. Looking at those invariants one can easily determine that although using similar variables, the 2 states differ.

It's crucial to note that my issue outlines a method by which a malicious user could exploit the vulnerability to freeze other users' funds or potentially lead to a loss of user funds, due to the arbitrarily selectable duration of the crowdfund. Unlike the situation described in the comment, a "permanent" loss of user funds would be possible in my case. According to the comment, the host can always close the funds after a sufficient number of deposits have been received. This is impossible in the state my issue describes, as minTotalContributions can never be reached, further differentiating the issues.

Additionally, I'd like to point out that among the issues raised by @ydspa in batch #552, only issue #127 has demonstrated how this vulnerability can be exploited and is a valid duplicate.

[c4-judge]

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

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[gzeon-c4]

@gzeon-c4 Thanks for taking a look at the issue. I'd like to emphasize that, in my perspective, this issue should not be considered out of scope based on the following arguments.

The rationale for categorizing this issue as out of scope is primarily linked to the comment in Line 247-254. However, it's crucial to note that the comment discusses a different scenario and severity. Specifically, the comment states, "In this scenario users will have to wait until the crowdfund expires or a host finalizes after minTotalContribution has been reached by calling finalize()". It's important to highlight that the host finalizing the crowdfund is not feasible in the context showcased and validated by the POC. In the case I've outlined, no host has the capability to prematurely close the fund and rescue user funds. This leads to a different threat than described by the comment, which incorrectly gives users the security of their funds being rescued by a host in the case of a crowdfund getting stuck.

Furthermore, the comment illustrates a situation where minTotalContributions can still be reached by depositing, while maxTotalContributions cannot be achieved. This state can be described as an invariant: minTotalContributions <= potentialDeposits < maxTotalContributions. In the case I've presented, neither of these thresholds can be reached. The invariant for my scenario reads as follows: potentialDeposits < minTotalContributions <= maxTotalContributions. Looking at those invariants one can easily determine that although using similar variables, the 2 states differ.

It's crucial to note that my issue outlines a method by which a malicious user could exploit the vulnerability to freeze other users' funds or potentially lead to a loss of user funds, due to the arbitrarily selectable duration of the crowdfund. Unlike the situation described in the comment, a "permanent" loss of user funds would be possible in my case. According to the comment, the host can always close the funds after a sufficient number of deposits have been received. This is impossible in the state my issue describes, as minTotalContributions can never be reached, further differentiating the issues.

Additionally, I'd like to point out that among the issues raised by @ydspa in batch #552, only issue #127 has demonstrated how this vulnerability can be exploited and is a valid duplicate.

Fair, judging as Med due to fund are only stuck but not lost in a reasonably configured party (expire after realistic duration)

[c4-judge]

gzeon-c4 marked the issue as satisfactory