Loss of assets when rageQuit due to front-running token distribution

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/06e6271b75a30f00ae2674e8c26f3eb9e89e75e5/contracts/party/PartyGovernance.sol#L500-L502
https://github.com/code-423n4/2023-10-party/blob/06e6271b75a30f00ae2674e8c26f3eb9e89e75e5/contracts/party/PartyGovernanceNFT.sol#L374-L405

Vulnerability details

Impact

Members face a risk of losing their assets if a rageQuit is front-ran by a token distribution. This vulnerability stems from the rageQuit function's dependency on the Party contract's token balance, which can be depleted by a distribution event.

Proof of Concept

In Party contract, there is a state when a member can make a loss to other members. Let's see the scenario:

(1) When creating distribution, if setting distributionsRequireVote equals false, then members are allow to create a token distribution by moving the party's entire balance to the TokenDistributor contract.

    // Must not require a vote to create a distribution, otherwise
    // distributions can only be created through a distribution
    // proposal.
    if (_getSharedProposalStorage().opts.distributionsRequireVote) {
        revert DistributionsRequireVoteError();
    }

https://github.com/code-423n4/2023-10-party/blob/06e6271b75a30f00ae2674e8c26f3eb9e89e75e5/contracts/party/PartyGovernance.sol#L500-L502

(2) When the party is allowed a member to rage quit, it essentially burn member's governance NFT and withdraw a fair share of fungible tokens from the party.

The withdrawn amount is calculated by the balance of tokens in Party contract. It then transferred the amount to the member and burn the NFT, and reduce the total voting power.

    // 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) {
            // Check if order of tokens to transfer is valid.
            // Prevent null and duplicate transfers.
            if (prevToken >= withdrawTokens[i]) revert InvalidTokenOrderError();

            prevToken = withdrawTokens[i];

            // Check token's balance.
            uint256 balance = address(withdrawTokens[i]) == ETH_ADDRESS
                ? address(this).balance
                : withdrawTokens[i].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.
                withdrawAmounts[i] += (balance * getVotingPowerShareOf(tokenIds[j])) / 1e18;
            }
        }
    }
    {
        // 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, not an authority, or if there are duplicate token IDs.
        uint96 totalVotingPowerBurned = _burnAndUpdateVotingPower(tokenIds, !isAuthority_);

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

https://github.com/code-423n4/2023-10-party/blob/06e6271b75a30f00ae2674e8c26f3eb9e89e75e5/contracts/party/PartyGovernanceNFT.sol#L374-L405

So a malicious user can observe a rageQuit transaction from a member and front-run it by calling distribute to create a distribution. Because the balance of tokens are transferred from Party contract to TokenDistributor contract, so the member who rageQuit will receive nothing and has his NFTs burn. Because his NFT is burnt, he cannot claim the token in TokenDistribution contract.

Tools Used

Manual

Recommended Mitigation Steps

Do not allow distribute to be call by member, it has to be through proposal voting and delay execution.

Assessed type

Timing

[c4-pre-sort]

ydspa marked the issue as duplicate of #547

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-pre-sort]

ydspa marked the issue as not a duplicate

[c4-pre-sort]

ydspa marked the issue as sufficient quality report

[c4-pre-sort]

ydspa marked the issue as duplicate of #529

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

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

[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 full credit

[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 duplicate of #529

[c4-judge]

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

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-c