rageQuit() maybe can't execute as it should

[c4-submissions]

Lines of code

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

Vulnerability details

Description

The PartyGovernanceNFT.rageQuit() is to burn the governance NFT of the user and withdraw a fair share of the selected ERC20 tokens and ETH from the party.
The users can specify the fungible tokens to withdraw (ERC 20)

However, after determining the overall amount to be withdrawn, if the distribution fee bps is greater than zero so feeBps > 0
The protocol will take a fee from the amount

PartyGovernanceNFT.sol

                if (fee > 0) {
                    amount -= fee;

                    // Transfer fee to fee recipient.
                    if (address(token) == ETH_ADDRESS) {
                        payable(feeRecipient).transferEth(fee);
                    } else {
                        token.compatTransfer(feeRecipient, fee);
                    }
                }

As the transferred token could be any ERC20 token If the distribution fee recipient feeRecipient is added to the USDC blacklist

by adding the feeRecipient address to the USDC or any token blacklist users will get forced to leave their shares of USDC in the party in order to rageQuit() successfully

It's important to be aware that addresses can be blacklisted by Circle at any given time MORE HERE

Impact

• The users will not be able to retrieve their share of the funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of sending tokens directly to the feeRecipient, consider storing the number of tokens in variables and having the feeRecipient claim it later.
This approach allows for better control. the feeRecipient can claim the tokens at a later point, ensuring a more organized and flexible transaction process.
or add the ability to change the address of feeRecipient

Assessed type

Other

[ydspa]

QA: L

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-pre-sort]

ydspa marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[Ch-301]

In order to use the rageQuit() functions the users are forced to lose their funds
and no one can update the feeRecipient address.

The likelihood is low but the impact is high
I believe the severity is medium

Thanks!

[gzeon-c4]

Bumping to QA, it is possible to fix by a governance proposal with arbitrary delegate call

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-b

[Ch-301]

@gzeon-c4
I'm wondering how an arbitrary delegate call could change the state variable feeRecipient in the Party that has no logic to do that (except the _initialize())?

[gzeon-c4]

@gzeon-c4 I'm wondering how an arbitrary delegate call could change the state variable feeRecipient in the Party that has no logic to do that (except the _initialize())?

Sorry, was wrong about that.
Still think the risk is Low (either misconfiguration, or being blacklisted which I think is mostly out-of-scope) but agree this can be a nice feature to have. @0xble

[0xble]

@gzeon-c4 I'm wondering how an arbitrary delegate call could change the state variable feeRecipient in the Party that has no logic to do that (except the _initialize())?

Sorry, was wrong about that. Still think the risk is Low (either misconfiguration, or being blacklisted which I think is mostly out-of-scope) but agree this can be a nice feature to have. @0xble

While it'd be better practice to use pull-over-push, our team has decided we generally don't opt to follow it when it hurts UX.

Agree this seems like a QA over Med.

[Ch-301]

...being blacklisted which I think is mostly out-of-scope...

@gzeon-c4 Did I miss any part from Readme or Docs talk about that?

I believe saving the user from sacrificing his funds in order to rageQuit() is not a feature, especially when no one can update the feeRecipient address.
So when it gets blacklisted this issue is there forever

[gzeon-c4]

Realistically there are infinite ways an external integration can rug this protocol, the risk is low. QA is the final decision.