rageQuit() can be front-ran by an attacker to cause permanent fund loss for rage-quitters and potentially profit off from that.

[c4-submissions]

Lines of code

https://github.com/PartyDAO/party-protocol/blob/37dae4292dde2547a3b1ced8a041f926e1b37d58/contracts/party/PartyGovernanceNFT.sol#L389

Vulnerability details

Pre-requisite knowledge & an overview of the features in question

---

1. The distributionsRequireVote flag: The distributionsRequireVote flag is a governance value flag set to false by default in the governance values. It determines whether or not a party member can simply create a distribution without the need to create a proposal, get it to pass, or not.

   • By default, it is set to as false because creating token distributions isn't seen as a sensitive action. If a party member creates a distribution, then everybody can simply claim his share of the distribution. No funds will be lost.

   • By default, the distribution creation without voting feature/flag is toggled on unlike the governance values like enableAddAuthorityProposal, allowArbCallsToSpendPartyEth, allowOperators. Those are flags you have to toggle on by creating a proposal and getting it to pass because they are seen as sensitive actions.

---

Overview of the vulnerability / PoC

---

There is no front-running protection on the rageQuit() function in PartyGovernanceNFT contract. If this is coupled with the fact that the governance values of the party allows creation of distributions without voting (distributionsRequireVote is set to false), this will allow an attacker each time someone ragequits to cause a permanent fund loss for the rage-quitter. If, for example, a party member wants to rage quit and he is supposed to get 10 ETH in exchange for burning his governance NFT tokens, he is going to get none and his governance NFT will be burnt so it's irreversible damage.

How the attack looks like

1. Suppose we have a party with the following values:

   1. A total voting power of 100e18
   2. An ETH balance of 100 ether

2. A party member who wants to rage quit has a governance NFT token with a voting power of 10e18. (10% of the total voting power).

3. The rage quitter wants to rage quit and get his claimable share in ETH balance. So he calls the rageQuit() function and asks the party contract to burn his governance NFT token with a voting power of 10e18 and get his share of the party's ETH balance, which will be 10% so he should get 10 ETH.

4. The attacker front-runs his transaction with a distribute() call to create a distribution of native ETH balance.

5. The distribute() function will transfer all of the party's ETH balance to the TokenDistributor. So there will be no remaining ETH balance in the Party contract.

6. The victim's rageQuit() function call/tx will start executing. It will retrieve the current balance of the party of the token/coin the ragequitter wants to claim, in this case it is ETH. The address(this).balance call will return 0.

7. Based on the result of the balance, it will conduct a mathematical operation to determine the ETH/ERC20 withdraw amount. https://github.com/PartyDAO/party-protocol/blob/37dae4292dde2547a3b1ced8a041f926e1b37d58/contracts/party/PartyGovernanceNFT.sol#L371

    // Check token's balance.
    uint256 balance = address(token) == ETH_ADDRESS
        ? address(this).balance
        : token.balanceOf(address(this));

    // Add fair share of tokens from the party to total.
    for (uint256 j; j < tokenIds.length; ++j) {
        // Must be retrieved before burning the token.
        uint256 shareOfVotingPower = getVotingPowerShareOf(tokenIds[j]);

        withdrawAmounts[i] += (balance * shareOfVotingPower) / 1e18;
    }

    // Sum up total amount of each token to withdraw.
    uint256[] memory withdrawAmounts = new uint256[](withdrawTokens.length);
    {
        IERC20 prevToken;
        for (uint256 i; i < withdrawTokens.length; ++i) {
            IERC20 token = withdrawTokens[i];

            // Check if order of tokens to transfer is valid.
            // Prevent null and duplicate transfers.
            if (prevToken >= token) revert InvalidTokenOrderError();

            prevToken = token;

            // Check token's balance.
            uint256 balance = address(token) == ETH_ADDRESS
                ? address(this).balance
                : token.balanceOf(address(this));

            // Add fair share of tokens from the party to total.
            for (uint256 j; j < tokenIds.length; ++j) {
                // Must be retrieved before burning the token.
                uint256 shareOfVotingPower = getVotingPowerShareOf(tokenIds[j]);

                withdrawAmounts[i] += (balance * shareOfVotingPower) / 1e18;
            }
        }
    }

8. (balance * shareOfVotingPower) / 1e18;. Since the remaining ETH balance in the party is zero, and since zero is still zero after being multiplied or divided by any other number. The total withdraw amount or in other words, the amount of ETH (or ERC20) the ragequitter will get is zero (he should've gotten 10 ETH).

9. Next, the rageQuit will burn the governance NFT tokens of the ragequitter and then decrease the total voting power of the party (will be decreased by 10% or 10e18). https://github.com/PartyDAO/party-protocol/blob/37dae4292dde2547a3b1ced8a041f926e1b37d58/contracts/party/PartyGovernanceNFT.sol#L385C1-L393C10

    {
        // Burn caller's party cards. This will revert if caller is not the
        // the owner or approved for any of the card they are attempting to
        // burn or if there are duplicate token IDs.
        uint96 totalVotingPowerBurned = _burnAndUpdateVotingPower(tokenIds, true);

        // Update total voting power of party.
        _governanceValues.totalVotingPower -= totalVotingPowerBurned;
    }

10. At this point, the ragequitter received zero ETH when he should've received 10 ETH and his intrinsic voting power among with his burnt governance NFT are completely gone. The attacker has caused permanent fund loss for the rage-quitting member.

11. Since the total voting power is decreased, that means that anytime ETH money moves to the party contract, an attacker will be able to rage-quit and leave with a share that is higher than if he hadn't executed his attack and the ragequitting party member received his fair share.

---

Impact

---

Allows a malicious member to cause permanent fund loss for other party members as well as potentially profit out of it (when totalVotingPower of the party decreases, the attacker's share of the party's assets increases).

---

Remediation

---

Can be fixed in two ways.

First method is to completely remove the distributionsRequireVote flag and require distributions to always be by proposal creation

Second method is to not allow a rageQuit() call to be executed in the same block as a distribute() call with the same token address the ragequitter is trying to withdraw.
If you want to go with this way of fixing the issue, you have to have some sort of "cooldown" mechanism between each distribute() call. Because if you didn't, then an attacker can keep causing a DoS for the ragequitter preventing him from ragequitting by just sending 1 wei to the party and creating a distribution of that 1 wei.

---

Assessed type

MEV

[c4-pre-sort]

ydspa marked the issue as sufficient quality report

[c4-pre-sort]

ydspa marked the issue as primary issue

[0xble]

Valid. In practice, distributionsRequireVote is always going to be enabled if rage quit is enabled in a Party. But perhaps we should enforce this at the protocol level.

[c4-sponsor]

0xble (sponsor) confirmed

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[Emedudu]

rageQuit allows the caller to specify the minWithdrawAmount he wants to receive.
If amount < minWithdrawAmount, call will revert
That is enough slippage protection.

[3docSec]

☝️ I agree with what @Emedudu wrote.
However, this is still a valid attack path because of #469. I have literally no idea of what this means for the sake of judging though 🧐

[Emedudu]

#469 is a valid issue💯,
But this issue is completely different from #469 and is invalid on its own as the attack scenario it is describing has already been mitigated by minWithdrawAmount.

[d3e4]

@gzeon-c4 This is the same as #383, which is OOS since it was already reported in code-423n4/2023-05-party-findings#22.

[romeroadrian]

@gzeon-c4 This is the same as #383, which is OOS since it was already reported in code-423n4/2023-05-party-findings#22.

Not only it was reported as you correctly state, but the slippage check (ie the fix) should also prevent this from happening (except for the detail reported in #469, which doesn't seem to be mentioned in this issue).

[gzeon-c4]

☝️ I agree with what @Emedudu wrote. However, this is still a valid attack path because of #469. I have literally no idea of what this means for the sake of judging though 🧐

Good point, I am going to group this under #469 despite a very similar issue was reported in a previous contest.

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as not selected for report

[c4-judge]

gzeon-c4 marked the issue as duplicate of #469

[c4-judge]

gzeon-c4 marked the issue as partial-50

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as partial-25

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

[gzeon-c4]

After more consideration, I think this is out-of-scope due being the same issue as code-423n4/2023-05-party-findings#22 and failure to mention the bypass #469

[stalinMacias]

Hey @gzeon-c4

#469 is by no means a "bypass" that the reporter forgot to mention. When the attacker executes his front-running attack, the following "if condition" won't be executed and therefore the comparison between "amount" and "minAmount" won't occur.

The if conditional won't be executed because the attacker would distribute all the assets and therefore the "amount" will be zero, and therefore "minAmount" won't even be checked.

Again, if we are being fair, the fact that the reporter didn't mention this, doesn't make this issue invalid/OOS. Since it's not a "bypass" and it doesn't affect anything and doesn't make this attack unexploitable or even less impactful.
-> Victim wants to rage quit, so he calls the rageQuit() function
-> Attacker front-runs TX with a call to distribute()
-> Victim will walk with zero funds and the function call will NOT revert, because the "amount < minAmount" comparison won't even occur because "amount" will NOT be greater than zero, please re-visit the code snippet.

Final point, the previously reported issue (code-423n4/2023-05-party-findings#9) in the previous contests just mentioned that if distribution proposals were to be executed just before a user ragequits, then the user will lose funds. So they were saying it's a "potential" attack and there needs to be a distribution proposal there and at a specific time there needs to be a user trying to rageQuit for this to work. And that's probably why the devs didn't fix this on code-level.

However, In this finding, the reporter clearly states that if distributionsRequireVote is disabled (default), this will allow an attacker to cause the permanent fund loss for the user without the need for a distribution proposal, no one ever mentioned this crucial detail.

Final point, the sponsor marked this issue as valid, "sponsor confirmed" and not "sponsor acknowledged" so it's clear that he intends to fully fix this and wasn't aware of the attack vector or at least wasn't aware of how serious it can be. This report should be credited for that and not dismissed because someone "hinted" at this in a previous contest while missing crucial details.

[Emedudu]

The report didn't directly mention the if(amount>0) bug as the root issue, but from the attack scenario described in the report, I think the warden spotted the bypass as the root issue.

It was stated that "So there will be no remaining ETH balance in the Party contract" (that means that address(this).balance==0, which will cause amount==0):

Hence, I think this should be a dup of #469

[stalinMacias]

Hey @Emedudu, thanks for the comment.

I don't think this one should be a duplicate of #469 because of the following:

1. While minAmount is ignored when amount is zero during rage quit #469 proposes a fix for this problem, it's not the only way to fix it and I proposed another way to fix it. The fact that a fix for a reported bug exists in another report that does not discuss the same exact issue, does not mean that both should be duplicates

2. minAmount is ignored when amount is zero during rage quit #469 doesn't really dive into issue this report discusses. minAmount is ignored when amount is zero during rage quit #469 is just stating that if the user rage-quits during the time where a distribution proposal is being executed, that will lead to fund loss.
   This report we're commenting on is discussing the fact that if the flag distributionsRequireVote is turned off (default), this will allow an attacker to front-run ANY rageQuit() call at ANY time causing permanent fund loss, basically allowing an attacker to cause permanent fund loss if anyone of the party members tries to rage quit at any given time. There doesn't have to be a distribution proposal. There doesn't have to be a user trying to rage-quit during the execution time window of a distribution proposal that has passed.

3. minAmount is ignored when amount is zero during rage quit #469 won't fix this issue if the user supplied a minimum amount of "0" for any of the tokens he is trying to withdraw. So even if the minAmount is ignored when amount is zero during rage quit #469 fix was implemented, the attacker can still execute this attack whenever the rage-quitter supplies a minAmount of 0 for any of the tokens he wants to withdraw and cause a permanent fund loss of the token that the user specified minAmount of 0 for.
   And there is nothing inherently wrong with supplying a minAmount of 0, so there is no reason for the user to shy away from supplying that value. Since the user is expecting to get his fair share of the funds regardless if he supplied a minAmount of 0 or a value > 0.

4. It makes sense to fix this issue using the fix proposed in this report rather than the ones in minAmount is ignored when amount is zero during rage quit #469. There is no reason for minAmount to be checked if there are zero funds in the party (amount = 0). But it definitely makes sense to prevent a user from rage-quitting in the same block as the distribute() function was called (proposed fix).

For the following 4 reasons, I don't think both reports should be treated as duplicates. @gzeon-c4

Thank you

[Emedudu]

Hello @stalinMacias ,

I still believe #469 is the root issue, and once it gets fixed, the scenario described in this report won't be possible.

While #469 proposes a fix for this problem, it's not the only way to fix it and I proposed another way to fix it. The fact that a fix for a reported bug exists in another report that does not discuss the same exact issue, does not mean that both should be duplicates

Both reports spotted a bug, and proposed different fixes. Isn't this why they should be dups?

This report we're commenting on is discussing the fact that if the flag distributionsRequireVote is turned off (default), this will allow an attacker to front-run ANY rageQuit() call at ANY time causing permanent fund loss, basically allowing an attacker to cause permanent fund loss if anyone of the party members tries to rage quit at any given time. There doesn't have to be a distribution proposal. There doesn't have to be a user trying to rage-quit during the execution time window of a distribution proposal that has passed.

The reason why attacker will be able to cause rageQuitter to lose funds is because he frontruns him with a Party#distribute call, which CAUSES THE TOKEN BALANCE OF THE CONTRACT TO BE 0, and allows the if(amount>0) bypass.
With the proposed fix in #469 : "In case that amount is zero - revert if the minimum amount is greater than zero", the rageQuitter's call will revert, so his NFT doesn't get burned, and he can claim his share of the distribution, meaning there is no loss of funds.

#469 won't fix this issue if the user supplied a minimum amount of "0" for any of the tokens he is trying to withdraw. So even if the #469 fix was implemented, the attacker can still execute this attack whenever the rage-quitter supplies a minAmount of 0 for any of the tokens he wants to withdraw and cause a permanent fund loss of the token that the user specified minAmount of 0 for.
And there is nothing inherently wrong with supplying a minAmount of 0, so there is no reason for the user to shy away from supplying that value. Since the user is expecting to get his fair share of the funds regardless if he supplied a minAmount of 0 or a value > 0.

The minWithdrawAmount serves as slippage protection for the rageQuitter, so if he inputs a minWithdrawAmount of 0, it means he is comfortable with receiving 0 or more tokens.(This is how slippage protections work)

It makes sense to fix this issue using the fix proposed in this report rather than the ones in #469. There is no reason for minAmount to be checked if there are zero funds in the party (amount = 0). But it definitely makes sense to prevent a user from rage-quitting in the same block as the distribute() function was called (proposed fix).

Although the proposed fix in this report will prevent the attack scenario described in this report, the mother bug(i.e. the bypass) won't get fixed, which could lead to problems in future as more functionalities get added.
The recommended fix in #469 however, is very simple and elegant, and once implemented, the mother bug gets fixed, and the attack scenario described in this report won't be possible.

IMO, #469 spotted and directly mentioned the root cause of this issue, while this report spotted the issue, but didn't give a clear/direct description, and that's why I think it should be a dup of #469

[gzeon-c4]

Thanks for all the inputs, those are very helpful.

While this report does set amount to 0, it is not an issue unless there is also the if(amount>0) bypass otherwise minWithdrawAmounts would make this un-exploitable. I have read the submission again and see no hint of the warden mentioning minWithdrawAmounts would be disregarded if the amount is 0, which therefore reduces this issue to code-423n4/2023-05-party-findings#22.

Some other arguments such as user not specifying minWithdrawAmounts (user error), distributionsRequireVote is set to false (configuration error) are out-of-scope. However I agree that allowing distributionsRequireVote while rage quit is active is undesirable, and since this led to a fix I am bumping the issue to Low/QA.

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)