Party DAO - chainsnake's results

Lines of code

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

Vulnerability details

Impact

If maxTotalContributions - minTotalContributions < minContribution in the ETH crowdfund options, and totalContributions becomes the value that meets the conditions totalContributions > maxTotalContributions - minContribution and totalContributions < minTotalContributions while crowdfunding, then the crowdfunding will have no way to win, so it will surely become lost.

Malicious hackers can run donation attacks for such initial crowdfundings to make the fundings lost without losing any of his ETHs(because after LOST, the attackers can retrieve ETHs by refunding).

Proof Of Concept

Let's assume the condition maxTotalContributions - minTotalContributions < minContribution meets in the ETH crowdfund options. While running crowdfunding, totalContributions becomes the value between minTotalContributions and maxTotalContributions - minContribution.

The following diagram will make it easier to understand the situation.

        (maxT - minCon)            minT                                          maxT
... -----------|--------*-----------|---------------------------------------------|
                    totalCon

maxT: maxTotalContributions
minT: minTotalContributions
minCon: minContributions
totalCon: totalContributions

In this situation, to make the initial funding win, more ETHs should be contributed and the amount should be greater than minContributions. If the amount is less, the contribution will fail by the following restriction in _processContribution() function.

        if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
        }

If we take the amount greater than minContributions, totalContributions + amount will be greater than maxTotalContributions, so the amount is subtracted by the following section in _processContribution() function.

        if (newTotalContributions >= maxTotalContributions_) {
            totalContributions = maxTotalContributions_;
            _finalize(maxTotalContributions_);
            uint96 refundAmount = newTotalContributions - maxTotalContributions;
            if (refundAmount > 0) {
                amount -= refundAmount; // --> amount is subtracted by refundAmount
                payable(msg.sender).transferEth(refundAmount);
            }
        ... ...

This subtraction makes amount less than minContributions, which is against the minimum contributions criteria.

In this situation, we can say the funding got FROZEN because it cannot be contributed, or running finalize() function will fail because the funding is still in active status. Thus, by all means, the crowdfunding won't be able to be saved, and after the expiry, it will become lost.

Here is a POC test case:

contract InitialETHCrowdfundPocTest is InitialETHCrowdfundTestBase {
    using Clones for address;

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

        address attacker = _randomAddress();
        vm.deal(attacker, 5 ether);

        // Attacker starts donation attack to make the crowdfunding LOST
        vm.startPrank(attacker);
        vm.expectEmit(true, false, false, true);
        emit Contributed(attacker, attacker, 4.5 ether, attacker);
        crowdfund.contribute{ value: 4.5 ether }(attacker, "");
        vm.stopPrank();

        address alice = _randomAddress();
        vm.deal(alice, 5 ether);

        // Moral contributor trying to contribute but always fails
        vm.startPrank(alice);
        vm.expectRevert(abi.encodeWithSelector(
            ETHCrowdfundBase.BelowMinimumContributionsError.selector,
            0.5 ether,
            1 ether
        ));
        crowdfund.contribute{ value: 2 ether }(alice, "");
        vm.stopPrank();
    }
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

There are two alternative options for mitigation.

Option 1: To make ETHCrowdfundBase contract allow small amount(less than minContributions) when the new total contribution exceeds maxTotalContributions. The implementation is to subtract refundAmount from amount after checking the minimum contribution.

+       uint96 refundAmount;
        if (newTotalContributions >= maxTotalContributions_) {
            totalContributions = maxTotalContributions_;
            _finalize(maxTotalContributions_);

            // Refund excess contribution.
-           uint96 refundAmount = newTotalContributions - maxTotalContributions;
+           refundAmount = newTotalContributions - maxTotalContributions;
            if (refundAmount > 0) {
-               amount -= refundAmount;
                payable(msg.sender).transferEth(refundAmount);
            }
        } else {
            totalContributions = newTotalContributions;
        }

        ... ...
        uint96 minContribution_ = minContribution;
        if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
        }
+       amount -= refundAmount;

Option 2: To update the validation of crowdfunding options in ETHCrowdfundBase::_initialize() function. The below update will make it remove the vulnerability.

-       if (opts.minTotalContributions > opts.maxTotalContributions) {
+       if (opts.minTotalContributions > opts.maxTotalContributions - opts.minContribution) {
            revert;
        }

Assessed type

Invalid Validation

[L-01] Should validate crowdfunding duration when initializing ETHCrowdfundBase

contracts/crowdfund/ETHCrowdfundBase.sol#L163

Currently, in ETHCrowdfundBase::_initialize, there is no validation for opts.duration, so this may result in LOST of the crowdfunding if the duration is zero or too short.

I suggest validating opts.duration with minimum duration criteria.

[L-02] PartyGovernance::_areVotesPassing() function returns true for vetoed proposals

contracts/party/PartyGovernance.sol#L1129-L1135

A voteCount of a vetoed proposal has a value of type(uint96).max. But as there is no consideration of vetoed voting count in _areVotesPassing(), the function will almost always return true for a vetoed proposal.

So I think there has to be a validation step in the function, which checks the vetoedness like the below:

    function _areVotesPassing(
        uint96 voteCount,
        uint96 totalVotingPower,
        uint16 passThresholdBps
    ) private pure returns (bool) {
+       if (voteCount == type(uint96).max) return false; // --> Add this validation
        return (uint256(voteCount) * 1e4) / uint256(totalVotingPower) >= uint256(passThresholdBps);
    }

[L-03] The redundant voting power should be returned in PartyGovernanceNFT::mint()

contracts/party/PartyGovernanceNFT.sol#L166-L202

If totalVotingPower is greater than zero and the sum of mintedVotingPower and votingPower to be minted is greater than totalVotingPower, there might exist redundant voting power which is not added to mintedVotingPower.

        if (totalVotingPower != 0 && totalVotingPower - mintedVotingPower_ < votingPower_) {
            unchecked {
                votingPower_ = totalVotingPower - mintedVotingPower_;
            }
        }

In this situation, in my opinion, the redundant voting power votingPower - votingPower_ should be sent back to the caller as a return value along with a new token Id so that the caller can do further operations like refunding extra contributions.

[L-04] OffChainSignatureValidator::isValidSignature() function reverts with the wrong error when it fails to recover the signer from the signature

contracts/signature-validators/OffChainSignatureValidator.sol#L58

ecrecover() returns address(0) when recovering fails due to like hashing wrong EIP-712 format and so on.

In this case, as there is no signature error check, now it reverts with NotMemberOfParty().

I suggest updating the function so that it reverts with the correct error like revert WrongFormat()

[NC-01] Checking length equality is required for member arrays of BatchContributeArgs and BatchContributeForArgs in InitialETHCrowdfund::batchContribute() and InitialETHCrowdfund::batchContributeFor()

Missing equality check of member arrays of args variable may cause out-of-range exception.

batchContribute(): contracts/crowdfund/InitialETHCrowdfund.sol#L204-L225 batchConttributeFor(): contracts/crowdfund/InitialETHCrowdfund.sol#L255-L273

[NC-02] Zero address(address(0)) check of contributor is missing in InitialETHCrowdfund::_contribute()

contracts/crowdfund/InitialETHCrowdfund.sol#L282-L285

[NC-03] Checking length equality is missing between withdrawTokens and minWithdrawAmounts in PartyGovernanceNFT::rageQuit()

Missing length equality check between withdrawTokens and minWithdrawAmounts variable may cause out-of-range exception while running rageQuit()

contracts/party/PartyGovernanceNFT.sol#L350-L366