ReraiseETHCrowdfund#claimMultiple can be used to grief large depositors

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-party/blob/440aafacb0f15d037594cebc85fd471729bcb6d9/contracts/crowdfund/ReraiseETHCrowdfund.sol#L333-L382

Vulnerability details

Impact

User can be grieved by being force minted a large number of NFTs with low voting power instead of one with high voting power

Proof of Concept

ReraiseETHCrowdfund.sol#L354-L377

    for (uint256 i; i < votingPowerByCard.length; ++i) {
        if (votingPowerByCard[i] == 0) continue;

        uint96 contribution = (votingPowerByCard[i] * 1e4) / exchangeRateBps;
        if (contribution < minContribution_) {
            revert BelowMinimumContributionsError(contribution, minContribution_);
        }

        if (contribution > maxContribution_) {
            revert AboveMaximumContributionsError(contribution, maxContribution_);
        }

        votingPower -= votingPowerByCard[i];

        // Mint contributor a new party card.
        uint256 tokenId = party.mint(contributor, votingPowerByCard[i], delegate);

        emit Claimed(contributor, tokenId, votingPowerByCard[i]);
    }

ReraiseETHCrowdfund#claimMultiple can be called by any user for any other user. The above loop uses the user specified votingPowerByCard to assign each token a voting power and mint them to the contributor. This is problematic because large contributors can have their voting power fragmented into a large number of NFTs which a small amount of voting power each. The dramatically inflates the gas costs of the affected user.

Example: minContribution = 1 and maxContribution = 100. User A contributes 100. This means they should qualify for one NFT of the largest size. However instead they can be minted 100 NFTs with 1 vote each.

Tools Used

Manual Review

Recommended Mitigation Steps

If msg.sender isn't contributor it should force the user to mint the minimum possible number of NFTs:

    uint256 votingPower = pendingVotingPower[contributor];

    if (votingPower == 0) return;

+  if (msg.sender != contributor) {
+      require(votingPowerByCard.length == (((votingPower - 1)/maxContribution) + 1));
+  }

[0xean]

Looking forward to sponsor comment on this one, I do see the potential issue.

[0xble]

To give more context, the reason we allow minting/claiming on another's behalf is to allow others to potentially unblock governance if a user with delegated voting power does not come to claim, so it is an important feature to enable. It also works this way in prior releases of the protocol with past crowdfunds.

This is valid concern though and I like the recommended mitigation.

[c4-sponsor]

0xble marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[0xble]

We've decided to refactor the way claiming works in the ReraiseETHCrowdfund, partially because a large number of findings like this being submitted around that one area that highlighted for us the need to rework its logic.

The change will make it so (1) crowdfund NFTs are minted per contribution instead of per address and (2) claiming works more like a 1:1 conversion of your crowdfund NFT into a party card instead of how it works now. In the future we will also add the ability to split/merge party cards.

This should mitigate this finding because in this new system you cannot decide how to allocate another person's voting power when claiming for them, there is only one choice which is to convert their crowdfund NFT into a party card of equivalent voting power.