In some cases the crowdfund cant be finalized because the minContribution amount check is after the amount is reduced

[c4-submissions]

Lines of code

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

Vulnerability details

Because of a finding in the previous contest, the minContribution check is done after the amount is potentially reduced if refunding excess contribution. However this can be a problem if the maxTotalContributions - minTotalContributions is smaller than the minContribution and in some cases the crowdfund can be lost because of this.

Example:

minTotalContributions = 100 ether
maxTotalContributions = 102 ether
The current totalContributions = 99 ether
minContribution amount = 4 ether

When the last contributor wants to contribute he calls the function with 4 ether however the reduced amount will then be 3 ether and the tx will revert because of the minContribution check and the crowdfund will be lost.

Impact

There will be no way to finalize the crowdfund and because of that it will be lost even though it was supposed to be finalized.

Proof of Concept

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

240:  amount -= refundAmount;

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

256:  if (amount < minContribution_) {
257:      revert BelowMinimumContributionsError(amount, minContribution_);
258:  }

As you can see the amount is reduced and the check is done after it, the problem arises when the maxTotalContributions - minTotalContributions is smaller than the minContribution which will cause the crowdfund to be lost.

Tools Used

Manual Review

Recommended Mitigation Steps

Because we cant move the check before the refund logic, the only way to prevent this is to check if the minContribution is bigger than the maxTotalContributions - minTotalContributions in the initialize function

Assessed type

Other

[ydspa]

Obviously it's a design choice, reference: https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L247-L254

Invalid: design decision

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-pre-sort]

ydspa marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

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

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked issue #127 as primary and marked this issue as a duplicate of 127

[gzeon-c4]

#453 (comment)