Users might lose funds after calling rageQuit() by malicious frontrunners.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-party/blob/f6f80dde81d86e397ba4f3dedb561e23d58ec884/contracts/party/PartyGovernanceNFT.sol#L293

Vulnerability details

Impact

Users might lose funds after rageQuit() if a malicious frontrunner takes out funds from the party by executing the proposal.

Proof of Concept

Users can burn their party NFTs and receive the share of the party's funds using rageQuit().

But this function doesn't have any protections like the slippage protection during the swap and the below scenario would be possible.

1. Let's assume rageQuitTimestamp = ENABLE_RAGEQUIT_PERMANENTLY so users can call rageQuit() anytime.
2. An honest user Alice is going to call rageQuit().
3. At that time, a distribution proposal(or other proposals that take out funds from the party) is executable.
4. After noticing that, a malicious user executes the proposal by frontrunning and transfers some funds from the party to the distributor.
5. After that, Alice's rageQuit() is executed and she can't claim the distributed funds as his NFT was burnt.

While discussing with the sponsor, he said We will warn users of rage quitting when there’s a proposal ready to execute and I think it's still dangerous because the proposal might be executable immediately in case of unanimousVotes.

    if (pv.passedTime != 0) {
        // Ready.
        if (pv.passedTime + gv.executionDelay <= t) {
            return ProposalStatus.Ready;
        }
        // If unanimous, we skip the execution delay.
        if (_isUnanimousVotes(pv.votes, pv.totalVotingPower)) {
            return ProposalStatus.Ready;
        }
        // Passed.
        return ProposalStatus.Passed;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should add the minAmountOut logic like the slippage protection to prevent an unexpected loss.

Assessed type

Other

[0xble]

Duplicate of #17

[c4-judge]

thereksfour marked the issue as primary issue

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-sponsor]

0xble marked the issue as sponsor confirmed

[0xble]

Will implement the recommended mitigation

[romeroadrian]

@thereksfour I've reported this in L-4 https://github.com/code-423n4/2023-05-party-findings/blob/main/data/adriro-Q.md

[c4-judge]

thereksfour marked the issue as selected for report

[d3e4]

In #22 I mentioned this issue as well:

The rage quitter cannot completely protect himself from this by calling claim() first, because the tokens might not yet have been distributed to the TokenDistributor until in a frontrun call to distribute() just before his rageQuit(). This way the rage quitter might be robbed of his fair share.

[thereksfour]

Different case, in #22, user lost funds in TokenDistributor, in this issue, user lost funds in PartyGovernanceNFT.

In #22 I mentioned this issue as well:

The rage quitter cannot completely protect himself from this by calling claim() first, because the tokens might not yet have been distributed to the TokenDistributor until in a frontrun call to distribute() just before his rageQuit(). This way the rage quitter might be robbed of his fair share.

[d3e4]

Different case, in #22, user lost funds in TokenDistributor, in this issue, user lost funds in PartyGovernanceNFT.

#9 and #17 are very explicit about this issue's being a rageQuit() frontrun by a distribution of funds. This is exactly what I reported in #22 in the quote above. It is the frontrunning which directly causes the loss of funds that you refer to ("This way the rage quitter might be robbed of his fair share").

[thereksfour]

Sorry, I still think these are two different issues, although there may be crossover in description, btw, I'll merge #9 into #22 if I accept your point

[d3e4]

Could you elucidate what you consider the two different issues to be? As I see it, one (#22, #34) is that rage quitting forfeits funds in TokenDistributor, the other (#9, #17) is that a rage quitter can be robbed of his funds in PartyGovernanceNFT by having them distributed in a frontrun to his rageQuit().

I very clearly discuss both of those issues in #22.
The first issue:

PartyGovernanceNFT.rageQuit() burns a governance NFT and transfers its share of the balance of ETH and tokens:

The problem with this is that the governance NFT might also have tokens to claim() in the TokenDistributor. These cannot be claimed after the governance NFT has been burned.

The second issue:

The rage quitter cannot completely protect himself from this by calling claim() first, because the tokens might not yet have been distributed to the TokenDistributor until in a frontrun call to distribute() just before his rageQuit(). This way the rage quitter might be robbed of his fair share.

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[thereksfour]

Sorry, wrong action

[thereksfour]

You are right, my previous understanding was incorrect.

Could you elucidate what you consider the two different issues to be? As I see it, one (#22, #34) is that rage quitting forfeits funds in TokenDistributor, the other (#9, #17) is that a rage quitter can be robbed of his funds in PartyGovernanceNFT by having them distributed in a frontrun to his rageQuit().

I very clearly discuss both of those issues in #22. The first issue:

PartyGovernanceNFT.rageQuit() burns a governance NFT and transfers its share of the balance of ETH and tokens:

The problem with this is that the governance NFT might also have tokens to claim() in the TokenDistributor. These cannot be claimed after the governance NFT has been burned.

The second issue:

The rage quitter cannot completely protect himself from this by calling claim() first, because the tokens might not yet have been distributed to the TokenDistributor until in a frontrun call to distribute() just before his rageQuit(). This way the rage quitter might be robbed of his fair share.

[c4-judge]

thereksfour marked the issue as not selected for report