PartyGovernanceNFT#decreaseTotalVotingPower() - Incorrect logic for setting totalVotingPower

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L255-L258

Vulnerability details

Impact

mintedVotingPower will become larger than totalVotingSupply in the Party leading to many errors like:

• unexpected behavior of the Party
• broken minting process (PartyGovernanceNFT.sol#mint())

          // `totalVotingPower - mintedVotingPower_` will revert due to underflow
          if (totalVotingPower != 0 && totalVotingPower - mintedVotingPower_ < votingPower_) {
              unchecked {
                  votingPower_ = totalVotingPower - mintedVotingPower_;
              }
          }

Explanation

The decreaseTotalVotingPower() function is responsible for decreasing the total voting power of the party.

    function decreaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
    }

Proof of Concept

The problem is that the decreaseTotalVotingPower() function does not enforce any checks that the Party new totalVotingPower' is less than the actual mintedVotingPower, which leads to a lot of errors in Party` contract behavior.

Coded Proof of Concept

Paste the following test in PartyGovernanceNFTTest:

    function test_decreaseTotalVotingPower_PoC() external {
        // 1. 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)
            })
        );

        // 2. Random Recipient get `60 voting power`
        address recipient = _randomAddress();
        vm.prank(address(partyAdmin));
        uint256 tokenId = party.mint(recipient, 60, recipient);
        assertEq(party.ownerOf(tokenId), recipient);
        assertEq(party.votingPowerByTokenId(tokenId), 60);
        assertEq(party.mintedVotingPower(), 60);

        // 3. Decrease Total Voting Power with `50` and the function does not revert
        uint96 votingPower = 50;
        address authority = address(partyAdmin);
        vm.prank(authority);
        party.decreaseTotalVotingPower(votingPower);

        // At he end `totalVotingPower == 50`, but `mintedVotingPower == 60`
        assertEq(party.getGovernanceValues().totalVotingPower, 50);
        assertEq(party.mintedVotingPower(), 60);
    }

Tools Used

• Manual Inspection

Recommended Mitigation Steps

Implement decreaseTotalVotingPower() function like this:

📁 File: PartyGovernanceNFT.sol

+   error TotalVotingPowerCannotBeLowerThanMintedVotingPower();

    function decreaseTotalVotingPower(uint96 votingPower) external {
        _assertAuthority();
        _getSharedProposalStorage()
            .governanceValues
            .totalVotingPower -= votingPower;

+      if (_getSharedProposalStorage().governanceValues.totalVotingPower < mintedVotingPower) {
+          revert TotalVotingPowerCannotBeLowerThanMintedVotingPower();
+      }
    }

Assessed type

Error

[ydspa]

Authority(InitialETHCrowdfund in this audit scope) is responsible to ensure voting powers at correct status, not mint/burn/ increase/decreaseVotingPower after party startup, and make rageQuit unavailable before crowd fund finalized

Invalid

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[radeveth]

Hi, @gzeon-c4. Thank you for the quick judging!

1. The comment from @ydspa is actually incorrect. Below is an excerpt from my discussion with Party DAO sponsors during the contest. The sponsors explicitly confirmed that the burn() function can be called both before and after the party has started, as in by the code.

2. I am convinced that the decreaseTotalVotingPower() function can also be called both before and after the party starts (it wouldn't make sense otherwise), and the code show this.

3. My report clearly describes and demonstrates how after mintedVotingPower become larger than totalVotingSupply, the future minting process will be broken and how this can lead to unexpected behavior in the Party. Additionally, a major invariant of the party - mintedVotingPower <= governanceValues.totalVotingPower might be broken.

4. From the code implementation, it is evident that nothing prevents the totalVotingSupply from decreasing to a value less than the mintedVotingPower.

Please review my report again.
I strongly believe that this issue should be mitigated to High Severity Issue.

[gzeon-c4]

It is judged as invalid / out-of-scope due to https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/README.md?plain=1#L93

• Authorities have “root user privileges” over a Party and is it well aware that they can exploit or break Parties in a wide variety of ways, especially if they are set to a malicious EOA or unsafe smart contract. This is why authorities are almost always smart contracts that are highly audited and trusted, and are NOT expected to be EOAs.

And the only supplied authority implementation is not vulnerable.

[radeveth]

It is judged as invalid / out-of-scope due to https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/README.md?plain=1#L93

• Authorities have “root user privileges” over a Party and is it well aware that they can exploit or break Parties in a wide variety of ways, especially if they are set to a malicious EOA or unsafe smart contract. This is why authorities are almost always smart contracts that are highly audited and trusted, and are NOT expected to be EOAs.

And the only supplied authority implementation is not vulnerable.

My report is not about malicious authorities. My report is about how mintedVotingPower can become larger than totalVotingSupply in the Party leading to many errors that I decribed in my report and comment above.

Also, the attack described in #414 comes from the fact that the decreaseTotalVotingPower() function does not enforce checking that totalVotingPower is not less than mintedVotingPower.

[radeveth]

Hey, @gzeon-c4!

Would really appreciate if you take look at my comment above.

[gzeon-c4]

According to the scope of this contest, I deemed that Authority contracts are expected to be audited so that they will not cause unintentional immediate or delayed side-effect.