The delegate of a contributor may be inconsist between crowdfund phase and party phase, causing unexpected delegate voting.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/ETHCrowdfundBase.sol#L201-L208
https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/InitialETHCrowdfund.sol#L295-L310
https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L194-L198

Vulnerability details

Impact

A contributor can change his existing delegate during crowdfund phase, expecting that the change will be propagated to the party phase automaticlly. But it's not true. One must issue another call to change his existing delegate during party phase. Before doing that, his delegate remains the old one, and his voting power may be used by the old delegate to vote for some proposals, which is not what he expects.

Proof of Concept

Let's assume Alice is a contributor and she delegates her voting power to Bob initially. After some time, before party phase, Alice wants to change her delegate to Charlie, so she makes another contribution with Charlie as the new delegate. What happens next is:

1. Alice's contribution is converted to voting power and delegate is updated to Charlie in _processContribution.

   File: contracts/crowdfund/ETHCrowdfundBase.sol#_contribute() => _processContribution()
   201:        address oldDelegate = delegationsByContributor[contributor];
   202:        if (msg.sender == contributor || oldDelegate == address(0)) {
   203:            // Update delegate.
   204:            delegationsByContributor[contributor] = delegate; // @audit: Update one's own existing non-null delegate.
   205:        } else {
   206:            // Prevent changing another's delegate if already delegated.
   207:            delegate = oldDelegate;
   208:        }

   https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/ETHCrowdfundBase.sol#L201-L208

2. Then Alice's voting power and her new delegate Charlie are expected to be propagated to party contract.

   • If voting power is 0, _contribute returns and new delegate is NOT propagated to party contract (InitialETHCrowdfund.sol#L298).
   • If token id is not 0, voting power is added to an existing token through increaseVotingPower, but new delegate is NOT propagated to party contract (InitialETHCrowdfund.sol#L307).
   • If token id is 0, voting power is added to a new minted token through _mint, and new delegate is propagated to party contract (InitialETHCrowdfund.sol#L302). But party contract override the new delegate with existing delegate (PartyGovernanceNFT.sol#L194-L198).

   File: contracts/crowdfund/InitialETHCrowdfund.sol#_contribute()
   295:        votingPower = _processContribution(contributor, delegate, amount);  // @audit: delegate updated.
   296:
   297:        // OK to contribute with zero just to update delegate.
   298:        if (amount == 0) return 0;  // @audit: amount == 0, will NOT propagate new delegate
   299:
   300:        if (tokenId == 0) {
   301:            // Mint contributor a new party card.
   302:            party.mint(contributor, votingPower, delegate); // @audit: propagate new delegate
   303:        } else if (disableContributingForExistingCard) {
   304:            revert ContributingForExistingCardDisabledError();
   305:        } else if (party.ownerOf(tokenId) == contributor) {
   306:            // Increase voting power of contributor's existing party card.
   307:            party.increaseVotingPower(tokenId, votingPower);  // @audit: tokenId != 0, will NOT propagate new delegate
   308:        } else {
   309:            revert NotOwnerError(tokenId);
   310:        }

   https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/InitialETHCrowdfund.sol#L295-L310

   File: contracts/party/PartyGovernanceNFT.sol#mint()
   194:        // Use delegate from party over the one set during crowdfund.
   195:        address delegate_ = delegationsByVoter[owner];
   196:        if (delegate_ != address(0)) {  // @audit: new delegate is ignored if delegate exists
   197:            delegate = delegate_;
   198:        }

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

3. When party phase starts (crowdfund finalized), Alice thinks her delegate is Charlie, but actually it's Bob. Alice still can change her delegate to Charlie by delegateVotingPower (L448-L453). But before Alice change her delegate, Bob holds her voting power and Bob can use that power to vote for existing proposals.

   448:    /// @notice Pledge your intrinsic voting power to a new delegate, removing it from
   449:    ///         the old one (if any).
   450:    /// @param delegate The address to delegating voting power to.
   451:    function delegateVotingPower(address delegate) external {
   452:        _adjustVotingPower(msg.sender, 0, delegate);
   453:    }

   https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernance.sol#L448-L453

Tools Used

Manual audit

Recommended Mitigation Steps

1. Remove the lines in mint to prevent the new delegate from been ignored. Since delegate is checked already in _processContribution, it can be used in mint directly.
2. Pass the updated delegate to mint. To be precise, use delegationsByContributor[contributor] instead of delegate to avoid invalid delegate.
3. Add a function named delegateVotingPowerWithAuthorities, similar to delegateVotingPower, and limit it to only authorities. If one updates delegate with 0 contribution, call the newly added delegateVotingPowerWithAuthorities before return.

Assessed type

Governance

[ydspa]

QA: NC

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[gzeon-c4]

Seems to be valid @0xble

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[arr00]

I view this as a dupe of #433 although it is slightly different.

[c4-sponsor]

arr00 (sponsor) confirmed

[arr00]

We plan on changing documentation to make it clear that the delegates are only for initial mints. We won't be changing the mint function as of now due to cascading issues with that.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[0xdeth]

Hello @gzeon-c4

This issue showcases the same attack as #418 and it should be a duplicate of it, not a separate issue.

Cheers.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #418

[c4-judge]

gzeon-c4 marked the issue as not selected for report