Rage quitter loses his claimable share of distributed tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

Rage quitter loses his claimable share of distributed tokens.

Proof of Concept

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

// Burn caller's party card. This will revert if caller is not the owner
// of the card.
burn(tokenId);

// Withdraw fair share of tokens from the party.
IERC20 prevToken;
for (uint256 j; j < withdrawTokens.length; ++j) {
    IERC20 token = withdrawTokens[j];

    // Prevent null and duplicate transfers.
    if (prevToken >= token) revert InvalidTokenOrderError();

    prevToken = token;

    // Check if token is ETH.
    if (address(token) == ETH_ADDRESS) {
        // Transfer fair share of ETH to receiver.
        uint256 amount = (address(this).balance * shareOfVotingPower) / 1e18;
        if (amount != 0) {
            payable(receiver).transferEth(amount);
        }
    } else {
        // Transfer fair share of tokens to receiver.
        uint256 amount = (token.balanceOf(address(this)) * shareOfVotingPower) / 1e18;
        if (amount != 0) {
            token.compatTransfer(receiver, amount);
        }
    }
}

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 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.

Recommended Mitigation Steps

Have rageQuit() call TokenDistributor.claim() before the governance NFT is burned.

Assessed type

Context

[thereksfour]

Similar issues are considered M in C4. Loss of funds requires external requirements (user misses call to claim() or frontrun occurs) Also, since the claim() only allows NFT owner to call, consider transferring the user's NFT to the contract in rageQuit().

    function claim(
        DistributionInfo calldata info,
        uint256 partyTokenId
    ) public returns (uint128 amountClaimed) {
        // Caller must own the party token.
        {
            address ownerOfPartyToken = info.party.ownerOf(partyTokenId);
            if (msg.sender != ownerOfPartyToken) {
                revert MustOwnTokenError(msg.sender, ownerOfPartyToken, partyTokenId);
            }
        }

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[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 acknowledged

[0xble]

We will warn users with unclaimed distributions on the frontend but will not make any code changes to enforce this.

[c4-judge]

thereksfour marked the issue as selected for report

[romeroadrian]

@thereksfour I think this issue should be more on the QA side as the sponsor clearly stated that rage quit may cause losses due to user inaction or mistake:

If a user intentionally or accidentally excludes a token in their ragequit, they forfeit that token and will not be able to claim it.

Similar to the described scenario, if a user forgets to call claim on the distributor before rage quit, they will lose their share of tokens.

[thereksfour]

In this issue, even if the user does not make any mistake, they may suffer a loss because there may be a potential frontrun attack.