Tokens with multiple entry points can lead to loss of funds in `rageQuit()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L323-L346

Vulnerability details

Tokens with multiple entry points can lead to loss of funds in rageQuit()

ERC20 tokens with multiple entry points (also known as double entry tokens or two address tokens) can be used to exploit the rageQuit() function and steal funds from the party.

Impact

The rageQuit() function can be used by a party member to exit their position and claim their share of tokens present in the party. The implementation takes an arbitrary array of tokens and transfers the corresponding amount of each to the given receiver:

https://github.com/code-423n4/2023-05-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L323-L346

323:             IERC20 prevToken;
324:             for (uint256 j; j < withdrawTokens.length; ++j) {
325:                 IERC20 token = withdrawTokens[j];
326: 
327:                 // Prevent null and duplicate transfers.
328:                 if (prevToken >= token) revert InvalidTokenOrderError();
329: 
330:                 prevToken = token;
331: 
332:                 // Check if token is ETH.
333:                 if (address(token) == ETH_ADDRESS) {
334:                     // Transfer fair share of ETH to receiver.
335:                     uint256 amount = (address(this).balance * shareOfVotingPower) / 1e18;
336:                     if (amount != 0) {
337:                         payable(receiver).transferEth(amount);
338:                     }
339:                 } else {
340:                     // Transfer fair share of tokens to receiver.
341:                     uint256 amount = (token.balanceOf(address(this)) * shareOfVotingPower) / 1e18;
342:                     if (amount != 0) {
343:                         token.compatTransfer(receiver, amount);
344:                     }
345:                 }
346:             }

The function correctly considers potential duplicate elements in the array. Line 328 validates that token addresses are in ascending order and that the address from the previous iteration is different from the address of the current iteration, effectively disallowing duplicates.

However, this isn't enough protection against tokens with multiple addresses. If the token has more than one entry point, then a bad actor can submit all of them to the rageQuit() function in order to execute the withdrawal multiple times, one for each available entry point. This will lead to loss of funds to other party members, as this vulnerability can be exploited by an attacker to withdraw more tokens than deserved.

Proof of concept

Let's assume there is a token with two entry points in address A and address B. The party holds 100 of these tokens and Alice, a party member, has an NFT corresponding to 50% of the total voting power.

Alice decides to exit her position by calling rageQuit(). Under normal circumstances she would get 50 tokens, as she has 50% of the share in the party. However, Alice calls rageQuit() passing both address A and address B in the withdrawTokens array. As these are different addresses, the implementation will consider the argument valid, and execute the withdrawal two times. For the first address, Alice will be transferred 50 tokens (100 50% = 50), and for the other address she will be transferred an additional of 25 tokens (50 50% = 25).

Recommendation

There is no easy solution for the issue. The usual recommendation in these cases is to implement a whitelist of supported tokens, but this will bring complexity and undermine the flexibility of the Party implementation.

Another alternative would be to first do a "snapshot" of available balances for the given tokens, and then, while executing the withdrawals, compare the current balance with the snapshot in order to detect changes:

1. Loop through each token and store the balance of the party contract, i.e. previousBalances[i] = withdrawTokens[i].balanceOf(address(this)).
2. Loop again through each token to execute withdrawals, but first check that the current balance matches the previous balance, i.e. require(previousBalances[i] == withdrawTokens[i].balanceOf(address(this))).

If balances don't match, this means that a previous withdrawal affected the balance of the current token, which may indicate and prevent a potential case of a token with multiple entry points.

Assessed type

Token-Transfer

[0xble]

Feel like this should be a QA, it can lead to loss of funds but only under the specific circumstance that a party holds a two address tokens which practically is very rare.

[c4-judge]

thereksfour marked the issue as primary issue

[thereksfour]

TrueUSD is an example and has a fairly high market volume: https://medium.com/chainsecurity/trueusd-compound-vulnerability-bc5b696d29e2

Similar issues can be found in https://github.com/code-423n4/2023-04-frankencoin-findings/issues/886

But the difference is that in #886 the attacker is actively depositing TrueUSD to make the attack, while this issue requires the victim to deposit TrueUSD, and then the attacker can get more TrueUSD, so I would consider this to be M

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[IAm0x52]

Worth mentioning up that the secondary entry to TrueUSD was disabled as a response to this type of vuln so this is no longer possible for it. Not sure if there are any other notable tokens that implement this pattern anymore

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-sponsor]

0xble marked the issue as sponsor acknowledged

[c4-judge]

thereksfour marked the issue as selected for report