PartyGovernanceNFT.sol#increaseTotalVotingPower - Malicious actor can front-run increaseTotalVotingPower() and rageQuit() by taking more funds from the Party and leaving less for the others

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The protocol offers enhanced functionality and flexibility to trusted actors (hosts) via the use of trusted smart contracts called authorities.

PartyGovernanceNFT#increaseTotalVotingPower() is one such function which is exclusively available to authorities. As the name implies, it is used to increase the total voting power of the party.

An authority which leverages that function is AddPartyCardsAuthority. It is an authority which atomically distributes new party cards and updates the total voting power as needed.

A malicious actor can front-run AddPartyCardsAuthority which calls increaseTotalVotingPower() and rageQuit() by taking more funds out of the Party than the rest of the users would be able to.

The reason is that increaseTotalVotingPower() dilutes the voting power of each user. Thus, when a user front-runs a call to increaseTotalVotingPower() they would be able to rage quit before their voting power is being diluted and would benefit from this fact by taking out more value from their Party tokens in comparison with the rest of the users.

Proof of Concept

The increaseTotalVotingPower() function is pretty straightforward - it simply adds votingPower to the totalVotingPower of the party:

function increaseTotalVotingPower(uint96 votingPower) external {
    _assertAuthority();
    _getSharedProposalStorage().governanceValues.totalVotingPower += votingPower;
}

Let's explore rageQuit(), which if enabled, allows users to burn a Party card and withdraw ETH and/or ERC20 tokens proportionally to the card's voting power:

withdrawAmounts[i] += (balance * getVotingPowerShareOf(tokenIds[j])) / 1e18;

As we can see in getVotingPowerShareOf(tokenId), the voting power share of a card is calculated by dividing it to the totalVotingPower of the Party, denominated in 1e18:

function getVotingPowerShareOf(uint256 tokenId) public view returns (uint256) {
    uint256 totalVotingPower = _getSharedProposalStorage().governanceValues.totalVotingPower;
    return
        totalVotingPower == 0 ? 0 : (votingPowerByTokenId[tokenId] * 1e18) / totalVotingPower;
}

Given the above, let's replace the voting power share formula in the calculation of withdraw amounts in rageQuit():

$$ Withdraw\ Amount = \frac{Balance \times Token\ Voting\ Power \times \cancel{1e18}}{Total\ Voting\ Power \times \cancel{1e18}} $$ $$ Withdraw\ Amount = \frac{Balance \times Token\ Voting\ Power}{Total\ Voting\ Power} $$

where

$Balance$ - the ETH/ERC20 balance of the Party

$Token\ Voting\ Power$ - the voting power of the token being burned during rageQuit()

$Total\ Voting\ Power$ - the total voting power of the Party

Let's take the following example.

Prerequisites:

• We have a party that only has ETH balance
• exchangeRateBps = 1e4, i.e. 1:1 conversion between ETH and voting power
• The party has rage quit enabled.

Example 1:

$ETH\ Balance = 15\ ETH = 15e18$

$Total\ Voting\ Power = 15e18$

$Alice's\ Token\ Voting\ Power = 5e18$

$Bob's\ Token\ Voting\ Power = 5e18$

1. The Party calls AddPartyCardsAuthority#addPartyCards() to mint additional cards worth an additional $5e18$ of voting power.
2. AddPartyCardsAuthority#addPartyCards() calls Party#increaseTotalVotingPower()
3. Alice front-runs the increaseTotalVotingPower() call and calls rageQuit() just before it with her total voting power of 5e18
4. Alice is able to withdraw $5\ ETH$:

$$ Alice's\ Withdraw\ Amount = \frac{\cancel{15e18} \times 5e18}{\cancel{15e18}} = 5e18 $$

This updates the Party state as follows:

$ETH\ Balance = 10e18$

$Total\ Voting\ Power = 10e18$

5. increaseTotalVotingPower(5e18) gets executed after Alice's rage quit which results in:

$ETH\ Balance = 10e18$

$Total\ Voting\ Power = 15e18$

6. Bob decides to rageQuit() as well. His withdraw amount is:

$$ Bob's\ Withdraw\ Amount = \frac{10e18 \times 5e18}{15e18} = 3.3e18 $$

You see how Alice and Bob had the same amount of voting power. But just because Alice was able to front run the increaseTotalVotingPower() she was able to not only take out more funds of the Party by rage quitting but this also impacted Bob in a negative way.

Now let's see what's going to happen if Alice doesn't front run increaseTotalVotingPower() and calls rageQuit after its execution.

Example 2

$ETH\ Balance = 15\ ETH = 15e18$

$Total\ Voting\ Power = 15e18$

$Alice's\ Token\ Voting\ Power = 5e18$

$Bob's\ Token\ Voting\ Power = 5e18$

1. The Party calls AddPartyCardsAuthority#addPartyCards() to mint additional cards worth an additional $5e18$ of voting power.
2. AddPartyCardsAuthority#addPartyCards() calls Party#increaseTotalVotingPower(5e18)

The Party state gets updated as follows:

$ETH\ Balance = 15e18$

$Total\ Voting\ Power = 20e18$

3. Alice calls rageQuit() with her total voting power of $5e18$. Her withdraw amount is 3.75e18:

$$ Alice's\ Withdraw\ Amount = \frac{15e18 \times 5e18}{20e18} = 3.75e18 $$

The Party state gets updated as follows:

$ETH\ Balance = 11.25e18$

$Total\ Voting\ Power = 15e18$

4. Bob calls rageQuit() with his total voting power. His withdraw amount is 3.75e18:

$$ Bob's\ Withdraw\ Amount = \frac{11.25e18 \times 5e18}{15e18} = 3.75e18 $$

To summarize, given all other parameters being equal, here is what the figures look like:

Alice front-runs increaseTotalVotingPower():

• Alice's withdrawal = $5e18$
• Bob's withdrawal = $3.3e18$

Alice doesn't front-run increaseTotalVotingPower():

• Alice's withdrawal = $3.75e18$
• Bob's withdrawal = $3.75e18$

POC

Note: The test test_rageQuitBeforeIncreaseTotalVotingPower showcases what's going to happen if the front running scenario occurs, i.e. Example 1.

Paste the following inside test/crowdfund/InitialETHCrowdfund.t.sol in contract InitialETHCrowdfundTest and run forge test --mt test_rageQuitBeforeIncreaseTotalVotingPower -vvvv:

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

    Party party = crowdfund.party();

    // Authority enables rageQuit
    vm.prank(address(this));
    party.setRageQuit(0x6b5b567bfe);

    // Alice contributes 5 ETH
    address alice = _randomAddress();
    vm.deal(alice, 5 ether);
    vm.prank(alice);
    crowdfund.contribute{value: 5 ether }(address(alice), "");

    // Bob contributes 5 ETH
    address bob = _randomAddress();
    vm.deal(bob, 5 ether);
    vm.prank(bob);
    crowdfund.contribute{value: 5 ether }(address(bob), "");

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

    // Make sure Party has a total balance and voting power of 15e18
    assertEq(party.getGovernanceValues().totalVotingPower, 15 ether);
    assertEq(address(party).balance, 15 ether);

    // At this point the Party calls AddPartyCardsAuthority#addPartyCards(5e18)
    // to mint additional cards worth of 5e18 voting power. Once the transaction
    // gets into the mempool, Alice sees it and front-runs it with a rageQuit() call
    // with her total voting power of 5e18.
    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = 1;
    IERC20[] memory withdrawTokens = new IERC20[](1);
    withdrawTokens[0] = IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
    uint256[] memory minWithdrawAmounts = new uint256[](1);
    minWithdrawAmounts[0] = 0;
    vm.prank(alice);
    party.rageQuit(tokenIds, withdrawTokens, minWithdrawAmounts, address(alice));

    // Alice manages to retrieve all of her 5 ETH
    // Note that due to rounding we don't get exactly 5e18, but the difference is negligible
    assertEq(address(alice).balance, 4999999999999999995);

    // At this point AddPartyCardsAuthority#addPartyCards(5e18) calls increaseTotalVotingPower(5e18)
    vm.prank(address(crowdfund));
    party.increaseTotalVotingPower(5 ether);

    // Bob rage quits
    tokenIds[0] = 2;
    vm.prank(bob);
    party.rageQuit(tokenIds, withdrawTokens, minWithdrawAmounts, address(bob));

    // Bob only gets 3.33e18 instead of 3.75e18
    // Note that due to rounding we don't get exactly 3333333333333333333, but the difference is negligible
    assertEq(address(bob).balance, 3333333333333333331);
}

Tools Used

Manual Review
Foundry

Recommended Mitigation Steps

The current best solution is enforcing the usage of a private mempool service for all authority actions.

Assessed type

Other

[c4-pre-sort]

ydspa marked the issue as duplicate of #545

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[0xdeth]

Hey @gzeon-c4

We don't see how this is invalid and intended behavior. The description and the PoC, of this issue, clearly showcase a massive difference in funds if the attack is executed successfully.

Also AddPartyCardsAuthority is used as an example, the protocol team has stated that authorities are smart contracts that the protocol team will vet and then users can use them.

Because the impact and the likelihood of this attack are both High, we argue that this issue and all other dupes of #545 are valid High severity findings.

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #414

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as not selected for report

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #545

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[gzeon-c4]

Hey @gzeon-c4

We don't see how this is invalid and intended behavior. The description and the PoC, of this issue, clearly showcase a massive difference in funds if the attack is executed successfully.

Also AddPartyCardsAuthority is used as an example, the protocol team has stated that authorities are smart contracts that the protocol team will vet and then users can use them.

Because the impact and the likelihood of this attack are both High, we argue that this issue and all other dupes of #545 are valid High severity findings.

See #414 (comment)
Also AddPartyCardsAuthority is out-of-scope