PartyGovernanceNFT#rageQuit() can lead to token loss for users when dealing with zero-balance ERC20 during a rageQuit()

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Calling Party's rageQuit() with a zero-balance ERC20 disregards the minWithdrawAmounts array, resulting in the loss of user tokens without compensation.

Proof of Concept

Consider the following scenario illustrating the issue:

1. A Party holds USDC tokens.
2. The entire USDC balance is transferred to another account, e.g., via a proposal.
3. Alice, unaware of the Party's depleted USDC holdings, attempts to rageQuit().
4. Alice specifies minimum withdrawal amounts using uint256[] minWithdrawAmounts.
5. Despite specifying minWithdrawAmounts, all of Alice's Party tokens are burned, and she receives nothing in return.

In PartyGovernanceNFT#rageQuit() the withdraw amount for each ERC20 is calculated as follows:

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

where

balance = uint256 balance = address(withdrawTokens[i]) == ETH_ADDRESS ? address(this).balance : withdrawTokens[i].balanceOf(address(this));

However, if the token is an ERC20 for which the Party does not have any remaining balance, withdrawAmounts[i] would be equal to 0.

This means the following check would not be executed at all:

if (amount > 0) { //@audit Would be skipped if amount is equal to 0.
    // ...
    // Check amount is at least minimum.
    if (amount < minAmount) { // This is never checked if amount is equal to 0.
        revert BelowMinWithdrawAmountError(amount, minAmount);
    }

    // ...
    payable(receiver).transferEth(amount);
}

Since the check for the minimum withdrawal amount is within the if statement, it is never executed when amount is equal to 0, leading to unintended NFT loss.

uint96 totalVotingPowerBurned = _burnAndUpdateVotingPower(tokenIds, !isAuthority_); 

// Update total voting power of party.
_getSharedProposalStorage().governanceValues.totalVotingPower -= totalVotingPowerBurned;

PoC:

// Add this to PartyGovernanceNFT.t.sol

function testRageQuitDoesNotHonorMinWithdrawAmountIsERC20BalanceIsZero() external {
    // Create party
    (Party party, , ) = partyAdmin.createParty(
        partyImpl,
        PartyAdmin.PartyCreationMinimalOptions({
            host1: address(this),
            host2: address(0),
            passThresholdBps: 5100,
            totalVotingPower: 100,
            preciousTokenAddress: address(toadz),
            preciousTokenId: 1,
            rageQuitTimestamp: 0,
            feeBps: 0,
            feeRecipient: payable(0)
        })
    );

	// Configure rage quit
    vm.prank(address(this));
    party.setRageQuit(uint40(block.timestamp) + 1);

	// Give alice 10 voting tokens out of 100
    address alice = makeAddr("alice");
    vm.prank(address(partyAdmin));
    uint256 tokenId = party.mint(alice, 10, alice);

	// An ERC20 in which the party previously had holdings
	// but now it's balance is == 0.
    IERC20[] memory tokens = new IERC20[](1);
    tokens[0] = IERC20(address(new DummyERC20()));

	// The minimum withdraw alice is willing to accept
    uint256[] memory minWithdrawAmounts = new uint256[](1);
    minWithdrawAmounts[0] = 1; // Notice it's a value >0 to demonstrate the non-working check

    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = tokenId;

    // Make sure the party has 0 balance
    DummyERC20(address(tokens[0])).deal(address(party), 0);

    // Before
    assertEq(party.votingPowerByTokenId(tokenId), 10); // alice has 10 voting tokens
    assertEq(tokens[0].balanceOf(alice), 0); // alice has 0 DummyERC20s

    vm.prank(alice);
    party.rageQuit(tokenIds, tokens, minWithdrawAmounts, alice); // This should revert

    // After
    assertEq(party.votingPowerByTokenId(tokenId), 0); // alice has lost her 10 voting tokens
    assertEq(tokens[0].balanceOf(alice), 0); // alice's dummyERC20 balance is still 0
    assertEq(party.getGovernanceValues().totalVotingPower, 90);
}

Recommended Mitigation Steps

To ensure that the BelowMinWithdrawAmountError check is performed even when amount is equal to 0, amount > 0 must be replaced with amount >= 0 in the rageQuit() function. Here's the modified code snippet:

--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -418,17 +418,17 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
                     // Transfer fee to fee recipient.
                     if (address(token) == ETH_ADDRESS) {
                         payable(feeRecipient).transferEth(fee);
                     } else {
                         token.compatTransfer(feeRecipient, fee);
                     }
                 }

-                if (amount > 0) {
+                if (amount >= 0) {
                     uint256 minAmount = minWithdrawAmounts[i];

                     // Check amount is at least minimum.
                     if (amount < minAmount) {
                         revert BelowMinWithdrawAmountError(amount, minAmount);
                     }

                     // Transfer token from party to recipient.

Assessed type

Invalid Validation

[c4-pre-sort]

ydspa marked the issue as duplicate of #469

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[0xdeth]

Hey @gzeon-c4

Can you consider selecting this issue for report instead of #469?

We believe that it is better for the following reasons:

• Contains a coded PoC.
• Contains a more detailed explanation of the issue and its impact
• Contains a more detailed mitigation

We understand that selecting an issue for report is subjective, so we will respect any decision you make.

Cheers!

[gzeon-c4]

Fair, also linking over the comment.
#469 (comment)

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-sponsor]

0xble (sponsor) confirmed