totalVotingPower can be reduced to 0, leading to multiple DOS scenarios

[c4-submissions]

Lines of code

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

Vulnerability details

Bug Description

The recent changes to the protocol introduced the functionality for an authority to reduce the totalVotingPower of the party using the function decreaseTotalVotingPower(). The function allows an authority to decrease the totalVotingPower by an arbitrary number, up to the current totalVotingPower. However, if the authority decreases the totalVotingPower to 0, it results in multiple issues.

Impact

1. Users Cannot Create New Proposals

Calls to the propose() function will revert every time, due to the propose() function calling the accept() function. The accept() function includes a call to _areVotesPassing() which will revert due to a division by 0 in the following line.

return (uint256(voteCount) * 1e4) / uint256(totalVotingPower) >= uint256(passThresholdBps);

This not only results in a loss of functionality but also grants authority the power to stop the creation of new proposals intentionally, leading to potential governance freezing.

2. OffChainValidator Functionality Denial-of-Service (DOS)

If totalVotingPower is set to 0 and thresholdBps is not 0 in the OffChainValidator, calls to isValidSignature() will revert. This is due to a division by zero in the following lines:

if (
thresholdBps == 0 ||
(signerVotingPowerBps > totalVotingPower &&
signerVotingPowerBps / totalVotingPower >= thresholdBps)
) {
    return IERC1271.isValidSignature.selector;
}

This vulnerability results in a denial-of-service (DOS) of the ERC1271 functionality.

3. No Ragequitting will be possible

If totalVotingPowergets set to 0, users will not be able to ragequit anymore. This is due to a call to getVotingPowerShareOf() in rageQuit()which then calculates:

function getVotingPowerShareOf(uint256 tokenId) public view returns (uint256) {
    uint256 totalVotingPower = _getSharedProposalStorage().governanceValues.totalVotingPower;
    return totalVotingPower == 0 ? 0 : (votingPowerByTokenId[tokenId] * 1e18) / totalVotingPower;
}

This calculation will lead to a division by 0, reverting everytime. This restriction denies users the ability to withdraw their funds and disassociate from the party.

4. Incorrect NFT SVGs will be rendered

The PartyNFTRender contract generates erroneous SVGs for NFTs when totalVotingPower is 0. The VotingPowerPercentage becomes "--" due to the generateVotingPowerPercentage() function returning "--" in cases where totalVotingPower is 0. This inaccuracy in information is then incorporated into the NFT's SVG, providing users with misleading data.

function generateVotingPowerPercentage(uint256 tokenId) private view returns (string memory) {
    Party party = Party(payable(address(this)));

    uint256 totalVotingPower = getTotalVotingPower();
    if (totalVotingPower == 0) {
        return "--";
    }

    ...
}

5. tokenURI() will return incorrect metadata for NFTs

If the totalVotingPowerget set to 0, the description in the NFTs metadata will state "This item represents membership in %partyName. Exact voting power will be determined when the crowdfund ends. Head to %generateExternalURL() to view the Party's latest activity." which would incorrectly indicates that the party has not yet started. This happens due to the function generateDescription() calling to the function hasPartyStarted() to determine if it should add a description for before or after the party start. hasPartyStarted() will return false if totalVotingPower is 0 leading to the incorrect description.

function hasPartyStarted() private view returns (bool) {
    return getTotalVotingPower() != 0;
}

Every NFT's where the requirement renderingMethod == RenderingMethod.FixedCrowdfund || (renderingMethod == RenderingMethod.ENUM_OFFSET && getCrowdfundType() == CrowdfundType.Fixed) does not hold will return an incorrect name in its metadata. The name in the metadata will be "Party Membership" due to the function generateName() using the function hasPartyStarted() to determine if the Voting power can already be calculated. Otherwise it just returns "Party Membership" as the name.

if (hasPartyStarted()) {
    return string.concat(generateVotingPowerPercentage(tokenId), "% Voting Power");
} else {
    return "Party Membership";
}

Additionally no NFT's metadata will contain any attributes due to the function hasPartyStarted() being used to determine if attributes should be added in tokenURI.

hasPartyStarted() ? string.concat('", "attributes": [', generateAttributes(tokenId), "]") : '"', 
"}"

6. Voting power of NFTs can arbitrarily be increased by Authority

When the totalVotingPower is set to 0, any authority can use increaseVotingPower() to increase the voting power of a single NFT as much as they want. This is due to the capping of the mintedVotingPower_to the totalVotingPower only being in place in the function if totalVotingPower is not 0, as one can see below.

if (totalVotingPower != 0 && totalVotingPower - mintedVotingPower_ < votingPower) {
    unchecked {
        votingPower = totalVotingPower - mintedVotingPower_;
    }
}

7. No distribution will be possible

If the totalVotingPower gets set to 0, users will not be able to recover their funds using a distribution. This is due to the distribute() function checking the totalVotingPower, and if the totalVotingPower is 0 assumes the governance has not yet started. You can see this from this snippet:

if (_getSharedProposalStorage().governanceValues.totalVotingPower == 0) {
    revert PartyNotStartedError();
}

Proof of Concept

In the following one can find multiple POCs, for the more complex of the described issues.

Governance

This POC shows the issues where users can propose no new proposals, are not able to ragequit, no distributes can be generated and the authority can increase NFT's voting power arbitrarily:

```solidity function testTotalVotingPowerBecomesZero() external { // ------------------- SETUP START ------------------- ( Party party, IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds ) = partyAdmin.createParty( partyImpl, PartyAdmin.PartyCreationMinimalOptions({ host1: address(this), host2: address(0), passThresholdBps: 5000, totalVotingPower: 100, preciousTokenAddress: address(toadz), preciousTokenId: 1, rageQuitTimestamp: 0, feeBps: 0, feeRecipient: payable(0) }) ); //Add another address address recipient = _randomAddress(); //Deal tokens to the party IERC20[] memory tokens = new IERC20[](1); tokens[0] = IERC20(address(new DummyERC20())); uint256[] memory minWithdrawAmounts = new uint256[](1); minWithdrawAmounts[0] = 0; uint96[] memory balances = new uint96[](1); balances[0] = uint96(_randomRange(10, type(uint96).max)); DummyERC20(address(tokens[0])).deal(address(party), balances[0]); //Mint Governance NFTs partyAdmin.mintGovNft(party, address(john), 50, address(john)); vm.prank(address(partyAdmin)); uint256 tokenId = party.mint(recipient, 50, recipient); //Check that minted voting power is 100 assertEq(party.mintedVotingPower(), 100); //------------------- SETUP END ------------------- //The voting power gets reduced to 0 vm.prank(address(partyAdmin)); party.decreaseTotalVotingPower(100); PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({ maxExecutableTime: 9999999999, proposalData: abi.encodePacked([0]), cancelDelay: uint40(1 days) }); // John tries proposing a proposal which will revert due to divison by Zero vm.expectRevert(); john.makeProposal(party, p1, 1); uint256[] memory tokenIds = new uint256[](1); tokenIds[0] = tokenId; //Now the other user tries to ragequit to regain his funds but it reverts vm.expectRevert(); vm.prank(address(recipient)); party.rageQuit(tokenIds, tokens, minWithdrawAmounts, recipient); //Now the user tries to use a distribute to reclaim his funds but it reverts vm.expectRevert(); vm.prank(address(recipient)); party.distribute(address(party).balance, ITokenDistributor.TokenType.Native, ETH_ADDRESS, 0); //The authority can now arbitrarily increase any NFT's voting power vm.prank(address(partyAdmin)); party.increaseVotingPower(1, 100_000_000_000); } ```

The POC can be run by adding it to the GovernanceNFT.t.sol file and running it using forge test -vvvv --match-test "testTotalVotingPowerBecomesZero"

OffChainValidator

This POC shows that the OffChainValidator will stop working when totalVotingPower is 0.

```solidity function testDOSifTotalVotingPowerIsZero() public { //------ SETUP START ------// //Set total votes of party to 0 party.decreaseTotalVotingPower(2002); // Set the signing threshold to 50% vm.prank(address(party)); offChainGlobalValidator.setSigningThresholdBps(500); //------- SETUP END -------// (bytes32 messageHash, bytes memory signature) = _signMessage( johnPk, "Hello World! nonce:1000" ); bytes memory staticCallData = abi.encodeWithSelector( IERC1271.isValidSignature.selector, messageHash, signature ); //Any calls will revert not as totalVotingPower is 0 vm.expectRevert("Division or modulo by 0"); vm.startPrank(address(0), address(0)); (bool success, bytes memory res) = address(party).staticcall(staticCallData); } ```

The POC can be run by adding it to the OffChainSignatureValidator.t.sol file and running it using forge test -vvvv --match-test "testDOSifTotalVotingPowerIsZero"

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by checking if the totalVotingPower would be reduced to 0 by a call to decreaseTotalVotingPower(), and if that is the case reverting. This can be done by adapting the function like this:

function decreaseTotalVotingPower(uint96 votingPower) external {
    _assertAuthority();
    //We don't need to check for totalVotingPower < votingPower as this would revert anyways due to underflow protection
    require(_getSharedProposalStorage().governanceValues.totalVotingPower != votingPower)
    _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
}

Assessed type

Math

[c4-pre-sort]

ydspa marked the issue as duplicate of #480

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

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

[gzeon-c4]

It is the party authority responsibility to not intentionally break critical invariant, but a safe guard can be nice @0xble