Lines of code

Vulnerability details

Bug Description

The recent implementation update empowers the authority to decrease the totalVotingPower arbitrarily using the decreaseTotalVotingPower() function. This authority-exclusive function allows the reduction of totalVotingPower by a specified amount.

One of the main invariants of the governance logic is, that at any time, besides the minting of party cards for the initial crowdfund, totalVotingPower >= mintedVotingPower. This is done because the decision if a vote has passed is done based upon (simplified) dividing the amount of votes by the totalVotingPower and checking if that percentage is above the percentage set as the voting threshold. So if totalVotingPower exceeds mintedVotingPower it leads to the voting system not working as intended.

The issue arises as the decreaseTotalVotingPower() function lacks a check to prevent the new totalVotingPower from falling below mintedVotingPower. If the authority inadvertently decreases totalVotingPower too much, users or groups without majority voting power could pass proposals. If the totalVotingPower is reduced to a value less than the voting power of a single user or group, they could unanimously pass proposals, bypassing the veto period where hosts could otherwise prevent the proposal execution.

Impact

Majority

If the authority unintentionally reduces totalVotingPower below mintedVotingPower, users or groups lacking sufficient votes to pass a proposal gain the ability to pass proposals at will. This allows malicious users to perform various harmful actions, such as adding themselves as authorities through the AddAuthorityProposal and subsequently using the burn() function to remove all other users from the party. This could lead to the unauthorized sale of NFTs and the misappropriation of party funds.

It’s worth noting that the execution delay provides a safeguard, allowing hosts to cancel a malicious proposal before execution.

Unanimous Passing

If the totalVotingPower gets decreased so far that it falls below the voting power of a single user/group, proposal can get unanimously passed due to the check for (totalVotes * 1e4) / totalVotingPower >= 0.9999e4 in the function _isUnanimousVotes(). This leads to the proposal ignoring the veto period and getting executed directly, without needing a single host to have accepted the proposal.

If, through his vulnerability, a user is able to unanimously pass proposals, he can steal all NFT’s of the party using a ArbitraryCallsProposal. The ArbitraryCallsProposal actually implements a safeguard against it being used to transfer NFT’s, by a majority vote. This is done by checking at the start which NFT’s the contract owns, and then checking again at the end and reverting in the case of a difference. Additionally it also checks for the proposals calldata not doing any other malicious calls using the function _isCallAllowed. Unfortunately these safeguards do not work in the case of an unanimous vote, due to them only being enforced if the flag isUnanimous is not set. If the total voting power gets reduced below a users minted voting power, that user can directly pass a ArbitraryCallsProposal without needing to wait for the execution delay and can then steal all the parties NFTs.

Users can scan all parties that they currently have voting powers in constantly for this edge case. If it happens, they then frontrun any call to increaseTotalVotingPower() and steal all the parties NFTs.

Proof of Concept

A demonstration of the described issues is provided in this POC. It showcases scenarios where totalVotingPower is decreased below mintedVotingPower, allowing non-majority proposals to pass. Subsequently, the total voting power is lowered to that of a single user, enabling them to directly pass any proposal, even with a minority share of the party’s voting power.

    function testDecreaseTotalVotingPowerFallsBelowMinted() external {
        PartyParticipant alice = new PartyParticipant();
        PartyParticipant danny = new PartyParticipant();

        (Party party, IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds) = partyAdmin.createParty(
            partyImpl,
            PartyAdmin.PartyCreationMinimalOptions({
                host1: address(this),
                host2: address(0),
                passThresholdBps: 5100,
                totalVotingPower: 90,
                preciousTokenAddress: address(toadz),
                preciousTokenId: 2,
                rageQuitTimestamp: 0,
                feeBps: 0,
                feeRecipient: payable(0)
            })
        );

        //Mint toadz #2 to the party
        toadz.mint(address(party));

        //Check if the party owns the toadz #2
        assertEq(toadz.ownerOf(2), address(party));

        //Mint 3 NFTs
        partyAdmin.mintGovNft(party, address(john), 30, address(john));
        partyAdmin.mintGovNft(party, address(danny), 30, address(danny));
        partyAdmin.mintGovNft(party, address(alice), 30, address(alice));

        //Check that minted voting power is 90
        assertEq(party.mintedVotingPower(), 90);

        //Now decreease voring power to 50
        address authority = address(partyAdmin);
        vm.prank(authority);
        party.decreaseTotalVotingPower(40);

        //Still no revert here
        assertEq(party.getGovernanceValues().totalVotingPower, 50);

        //Now john starts a proposal
        PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({
            maxExecutableTime: 9999999999,
            proposalData: abi.encodePacked([0]),
            cancelDelay: uint40(1 days)
        });
        uint256 proposalid1 = john.makeProposal(party, p1, 1);

        _assertProposalStatus(party, proposalid1, PartyGovernance.ProposalStatus.Passed, 30);
        //So now john was able to pass a proposal although he has only 30 voting power and 51% of 90 minted should be needed

        //We wait for the veto period
        vm.warp(block.timestamp + 300);

        //If the hosts didn't see the issue, and veto, john could now execute
        _assertProposalStatus(party, proposalid1, PartyGovernance.ProposalStatus.Ready, 30);

        //Now we decrease the total voting power to 30
        vm.prank(authority);
        party.decreaseTotalVotingPower(20);

        assertEq(party.getGovernanceValues().totalVotingPower, 30);

        //Now john starts another proposal
        uint256 proposalid2 = john.makeProposal(party, p1, 2);

        //The proposal should be ready instantly as now the veto period gets skipped as it is unaimously accepted
        _assertProposalStatus(party, proposalid2, PartyGovernance.ProposalStatus.Ready, 30);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, it is recommended to include a check in the decreaseTotalVotingPower() function to ensure that the new totalVotingPower does not fall below mintedVotingPower. This mitigation step involves adapting the function to the following code snippet:

function decreaseTotalVotingPower(uint96 votingPower) external {
	_assertAuthority();
	require(_getSharedProposalStorage().governanceValues.totalVotingPower - votingPower >= mintedVotingPower, "Total voting power should never fall below minted voting power")
	_getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
}

Assessed type

Governance

---

The text was updated successfully, but these errors were encountered:

All reactions