PartyDAO contest - Lambda's results

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L594

Vulnerability details

Impact

PartyGovernanceNFT uses the voting power at the time of proposal when calling accept. The problem with that is that a user can vote, transfer the NFT (and the voting power) to a different wallet, and then vote from this second wallet again during the same block that the proposal was created. This can also be repeated multiple times to get an arbitrarily high voting power and pass every proposal unanimously.

The consequences of this are very severe. Any user (no matter how small his voting power is) can propose and pass arbitrary proposals animously and therefore steal all assets (including the precious tokens) out of the party.

Proof Of Concept

This diff shows how a user with a voting power of 50/100 gets a voting power of 100/100 by transferring the NFT to a second wallet that he owns and voting from that one:

--- a/sol-tests/party/PartyGovernanceUnit.t.sol
+++ b/sol-tests/party/PartyGovernanceUnit.t.sol
@@ -762,6 +762,7 @@ contract PartyGovernanceUnitTest is Test, TestUtils {
         TestablePartyGovernance gov =
             _createGovernance(100e18, preciousTokens, preciousTokenIds);
         address undelegatedVoter = _randomAddress();
+        address recipient = _randomAddress();
         // undelegatedVoter has 50/100 intrinsic VP (delegated to no one/self)
         gov.rawAdjustVotingPower(undelegatedVoter, 50e18, address(0));
 
@@ -772,38 +773,13 @@ contract PartyGovernanceUnitTest is Test, TestUtils {
         // Undelegated voter submits proposal.
         vm.prank(undelegatedVoter);
         assertEq(gov.propose(proposal, 0), proposalId);
-
-        // Try to execute proposal (fail).
-        vm.expectRevert(abi.encodeWithSelector(
-            PartyGovernance.BadProposalStatusError.selector,
-            PartyGovernance.ProposalStatus.Voting
-        ));
-        vm.prank(undelegatedVoter);
-        gov.execute(
-            proposalId,
-            proposal,
-            preciousTokens,
-            preciousTokenIds,
-            "",
-            ""
-        );
-
-        // Skip past execution delay.
-        skip(defaultGovernanceOpts.executionDelay);
-        // Try again (fail).
-        vm.expectRevert(abi.encodeWithSelector(
-            PartyGovernance.BadProposalStatusError.selector,
-            PartyGovernance.ProposalStatus.Voting
-        ));
-        vm.prank(undelegatedVoter);
-        gov.execute(
-            proposalId,
-            proposal,
-            preciousTokens,
-            preciousTokenIds,
-            "",
-            ""
-        );
+        (, PartyGovernance.ProposalStateValues memory valuesPrev) = gov.getProposalStateInfo(proposalId);
+        assertEq(valuesPrev.votes, 50e18);
+        gov.transferVotingPower(undelegatedVoter, recipient, 50e18); //Simulate NFT transfer
+        vm.prank(recipient);
+        gov.accept(proposalId, 0);
+        (, PartyGovernance.ProposalStateValues memory valuesAfter) = gov.getProposalStateInfo(proposalId);
+        assertEq(valuesAfter.votes, 100e18);
     }

Recommended Mitigation Steps

You should query the voting power at values.proposedTime - 1. This value is already finalized when the proposal is created and therefore cannot be manipulated by repeatedly transferring the voting power to different wallets.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L147

Vulnerability details

Impact

If opts.initialContributor is set to address(0) (and opts.initialDelegate is not), there are two problems: 1.) If the crowdfund succeeds, the initial balance will be lost. It is still accredited to address(0), but it is not retrievable. 2.) If the crowdfund does not succeed, anyone can completely drain the contract by repeatedly calling burn with address(0). This will always succeed because CrowdfundNFT._burn can be called multiple times for address(0). Every call will cause the initial balance to be burned (transferred to address(0)).

Issue 1 is somewhat problematic, but issue 2 is very problematic, because all funds of a crowdfund are burned and an attacker can specifically set up such a deployment (and the user would not notice anything special, after all these are parameters that the protocol accepts).

Proof Of Concept

This diff illustrates scenario 2, i.e. where a malicious deployer burns all contributions (1 ETH) of contributor. He loses 0.25ETH for the attack, but this could be reduced significantly (with more burn(payable(address(0))) calls:

--- a/sol-tests/crowdfund/BuyCrowdfund.t.sol
+++ b/sol-tests/crowdfund/BuyCrowdfund.t.sol
@@ -36,9 +36,9 @@ contract BuyCrowdfundTest is Test, TestUtils {
     string defaultSymbol = 'PBID';
     uint40 defaultDuration = 60 * 60;
     uint96 defaultMaxPrice = 10e18;
-    address payable defaultSplitRecipient = payable(0);
+    address payable defaultSplitRecipient = payable(address(this));
     uint16 defaultSplitBps = 0.1e4;
-    address defaultInitialDelegate;
+    address defaultInitialDelegate = address(this);
     IGateKeeper defaultGateKeeper;
     bytes12 defaultGateKeeperId;
     Crowdfund.FixedGovernanceOpts defaultGovernanceOpts;
@@ -78,7 +78,7 @@ contract BuyCrowdfundTest is Test, TestUtils {
                     maximumPrice: defaultMaxPrice,
                     splitRecipient: defaultSplitRecipient,
                     splitBps: defaultSplitBps,
-                    initialContributor: address(this),
+                    initialContributor: address(0),
                     initialDelegate: defaultInitialDelegate,
                     gateKeeper: defaultGateKeeper,
                     gateKeeperId: defaultGateKeeperId,
@@ -111,40 +111,26 @@ contract BuyCrowdfundTest is Test, TestUtils {
     function testHappyPath() public {
         uint256 tokenId = erc721Vault.mint();
         // Create a BuyCrowdfund instance.
-        BuyCrowdfund pb = _createCrowdfund(tokenId, 0);
+        BuyCrowdfund pb = _createCrowdfund(tokenId, 0.25e18);
         // Contribute and delegate.
         address payable contributor = _randomAddress();
         address delegate = _randomAddress();
         vm.deal(contributor, 1e18);
         vm.prank(contributor);
         pb.contribute{ value: contributor.balance }(delegate, "");
-        // Buy the token.
-        vm.expectEmit(false, false, false, true);
-        emit MockPartyFactoryCreateParty(
-            address(pb),
-            address(pb),
-            _createExpectedPartyOptions(0.5e18),
-            _toERC721Array(erc721Vault.token()),
-            _toUint256Array(tokenId)
-        );
-        Party party_ = pb.buy(
-            payable(address(erc721Vault)),
-            0.5e18,
-            abi.encodeCall(erc721Vault.claim, (tokenId)),
-            defaultGovernanceOpts
-        );
-        assertEq(address(party), address(party_));
-        // Burn contributor's NFT, mock minting governance tokens and returning
-        // unused contribution.
-        vm.expectEmit(false, false, false, true);
-        emit MockMint(
-            address(pb),
-            contributor,
-            0.5e18,
-            delegate
-        );
-        pb.burn(contributor);
-        assertEq(contributor.balance, 0.5e18);
+        vm.warp(block.timestamp + defaultDuration + 1);
+        // The auction was not won, we can now burn all ETH from contributor...
+        assertEq(address(pb).balance, 1.25e18);
+        pb.burn(payable(address(0)));
+        assertEq(address(pb).balance, 1e18);
+        pb.burn(payable(address(0)));
+        assertEq(address(pb).balance, 0.75e18);
+        pb.burn(payable(address(0)));
+        assertEq(address(pb).balance, 0.5e18);
+        pb.burn(payable(address(0)));
+        assertEq(address(pb).balance, 0.25e18);
+        pb.burn(payable(address(0)));
+        assertEq(address(pb).balance, 0);

Recommended Mitigation Steps

Do not allow an initial contribution when opts.initialContributor is not set.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L131 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L386

Vulnerability details

Impact

TokenDistributor.createERC20Distribution can be used to create token distributions for ERC777 tokens (which are backwards-compatible with ERC20). However, this introduces a reentrancy vulnerability which allows a party to get the tokens of another party. The problem is the tokensToSend hook which is executed BEFORE balance updates happens (see https://eips.ethereum.org/EIPS/eip-777). When this hook is executed, token.balanceOf(address(this)) therefore still returns the old value, but _storedBalances[balanceID] was already decreased.

Proof Of Concept

Party A and Party B have a balance of 1,000,000 tokens (of some arbitrary ERC777 token) in the distributor. Let's say for the sake of simplicity that both parties only have one user (user A in party A, user B in party B). User A (or rather his smart contract) performs the following attack:

• He calls claim, which transfers 1,000,000 tokens to his contract address. In _transfer, _storedBalances[balanceId] is decreased by 1,000,000 and therefore now has a value of 1,000,000.
• In the tokensToSend hook, he initiates another distribution for his party A by calling PartyGovernance.distribute which calls TokenDistributor.createERC20Distribution (we assume for the sake of simplicity that the party does not have more of these tokens, so the call transfers 0 tokens to the distributor). TokenDistributor.createERC20Distribution passes token.balanceOf(address(this)) to _createDistribution. Note that this is still 2,000,000 because we are in the tokensToSend hook.
• The supply of this distribution is calculated as (args.currentTokenBalance - _storedBalances[balanceId]) = 2,000,000 - 1,000,000 = 1,000,000.
• When the tokensToSend hook is exited (and the first transfer has finished), he can retrieve the tokens of the second distribution (that was created in the hook) to steal the 1,000,000 tokens of party B.

Recommended Mitigation Steps

Do not allow reentrancy in these functions.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L142

Vulnerability details

Impact

ArbitraryCallsProposal._isCallAllowed does not disallow ERC20 transfers and calls to Fractional. This can be exploited in combination with a FractionalizeProposal to get a precious token without an unanimous vote, which should not be possible.

Proof Of Concept

There are two different attack possibilities:

Transferring Fractionalize tokens out

• A user with 51% voting rights creates a FractionalizeProposal that succeeds, the party gets all of the ERC20 Fractionalize tokens.
• Afterwards, he creates an ArbitraryCallsProposal to transfer all ERC20 Fractionalize to a wallet that he owns.
• Now, he can call redeem on Fractionalize (https://github.com/fractional-company/contracts/blob/master/src/ERC721TokenVault.sol#L371) to get the NFT back.

ArbitraryCallsProposal to call redeem

• A user with 51% voting rights creates a FractionalizeProposal that succeeds, the party gets all of the ERC20 Fractionalize tokens.
• Afterwards, he creates an ArbitraryCallsProposal that does two things:
  • redeem is called on Fractionalize (https://github.com/fractional-company/contracts/blob/master/src/ERC721TokenVault.sol#L371), the NFT is transferred back to the party
  • The user transfers the NFT to his wallet.

Note that this attack succeeds because the party was not in possession of the NFT before the ArbitraryCallsProposal was executed, meaning that it will not be checked if it is in possesion after the execution.

Recommended Mitigation Steps

For attack 1, disallow ERC20 transfers (this should be done via the TokenDistributor). For attack 2, disallow calls to the Fractionalize vault via an ArbitraryCallsProposal.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L333

Vulnerability details

Impact

The hash of the governance options is truncated to 16 bytes instead of using the full 32 bytes hash. Because of that, there are 2^128 different hash values and finding a collision takes 2^127 tries on average, i.e. a reduction by factor 2^128. While this nowadays still takes a long time, this may change in the future. The consequences of finding a collision would be severe. An attacker could provide a hosts array that contains an address that he controls and create the party with this array, giving him voting power (and the ability to remove the host status of all other hosts). Furthermore, he could set himself as the feeRecipient to get all fees.

Proof Of Concept

We assume that in the future, an attacker with access to a supercomputer can calculate 2^110 hashes per second. The attacker fixes the first address of hosts to himself (and sets the other struct options to some values he desires, e.g. high fees and himself as the recipient). The second address of hosts is used by him to bruteforce the hashes, i.e. he simply tries different addresses there to get the desired hash.

This attack would take ~36 hours in expectation. If the full hash would be used, it would take over 10^36 years, i.e. 10^26 times the age of the universe.

Recommended Mitigation Steps

Use the full 32 bytes hash.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L370 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1066

Vulnerability details

Impact

Currently, the votingPower calculation rounds down for every user except the splitRecipient. This can cause situations where 99.99% of votes (i.e., an unanimous vote) are not achieved, even if all vote in favor of a proposal. This can be very bad for a party, as certain proposals (transferring precious tokens out) require an unamimous vote and are therefore not executable.

Proof Of Concept

Let's say for the sake of simplicity that 100 persons contribute 2 wei and splitBps is 10 (1%). votingPower for all users that contributed will be 1 and 2 for the splitRecipient, meaning the maximum achievable vote percentage is 102 / 200 = 51%.

Of course, this is an extreme example, but it shows that the current calculation can introduce siginificant rounding errors that impact the functionality of the protocol.

Recommended Mitigation Steps

Instead of requiring more than 99.99% of the votes, ensure that the individual votingPower sum to the total contribution. For instance, one user (e.g., the last one to claim) could receive all the remaining votingPower, which would require a running sum of the already claimed votingPower.

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L170

Vulnerability details

Impact

CrowdfundNFT.bid is callable by anyone. This design enables and incentivizes a seller to always sell his NFT for the maximum possible amount (wrt to the crowdfund), which hurts users of the Party protocol, as they pay potentially more than if they would not use the protocol.

Proof Of Concept

There is a Crowdfund to bid on a bored ape with a maximumBid of 100 ETH and the crowdfund has accumulated 100 ETH. We assume that the platform has a minimum increment of 10%. The seller of the bored ape can now do the following:

• Get a flash loan for ~90.9090 ETH
• Bid ~90.9090 ETH for the ape
• Call bid() on the Crowdfund, the crowdfund will now bid 100 ETH
• Get back the ~90.9090 ETH bid and pay back the flash loan.

Like that, the seller can always sell his NFT risk-free and without locking up capital for min(maximumBid, address(crowdfund).balance).

Recommended Mitigation Steps

Only allow to call bid if the user has participated in the crowdsale (with a minimum threshold to avoid that a seller participates with 1 wei). While this does not eliminate this issue completely, it is no longer completely risk-free and the seller has to lock up his capital: If no user calls bid(), the sale will not succeed from the sellers perspective, although it might have if he had not placed this bid by himself.

• In PartyGovernance._getProposalStatus, the constant VETO_VALUE is not used (https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1033), whereas it is used when performing the veto (https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L634). While this is not a problem currently (the values are identical), it can be dangerous when this value is updated at one point, because updating _getProposalStatus might get forgotten. Consider using the constant consistently.
• The link for FractionalizeProposal (https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/FractionalizeProposal.sol#L30) is wrong. It links to the source code of the vault, but the variable points to the factory. The correct link is https://github.com/fractional-company/contracts/blob/master/src/ERC721VaultFactory.sol
• After a FractionalizeProposal was executed succesfully and the tokens were distributed, it will not be possible to get the NFT back by calling redeem in practice. This function requires that the sender (the party in this case) owns all tokens, which would require that all users transfer them back again. However, transferring them to the party would be very risky when you do not if all other participants also do that (as the tokens are lost when one user does not transfer them) and some users may have sold them. This behavior may be desired (although it is a bit against the general philosophy were the tokens are protected IMO), but it should be documented that this is a destructive action.
• For Nouns, it is only possible to deploy the auction when the desired ID is for sale. However, it might be desired to create an auction in advance (e.g., for token ID 7 when the current ID is 5), such that there is more time to collect funds. For a system like Nouns this would work nicely, because it is (roughly) known in advance when an auction for a token will start.
• CrowdfundNFT, the implementation of a soulbound NFT, is a bit too simplistic in my opinion. One problem with soulbound NFTs is wallet recovery. In such situations, you want to be able to transfer the NFT to another wallet (but of course, this should not be possible without prior checks, otherwise they would not be soulbound). There are different standards like EIP 4671 that address soulbound NFTs (and the mentioned problems), consider implementing one of them.
• FoundationMarketWrapper.auctionIdMatchesToken returns true for auctionId 0 and isFinalized also returns true. Because of that, it is possible to create an auction with ID 0 where no one can bid because it is immediately finalized.
• ZoraMarketWrapper.auctionIDMatchesToken returns true for auctionId 0 and nftContract = address(0), and isFinalized als returns true. Because of that, it is possible to create an auction with ID 0 and address(0) where no one can bid because it is immediately finalized.
• BuyCrowdfund cannot retrieve ETH, but certain contracts that are called by it (e.g., NFT marketplaces) might return additional ETH when too much is paid. In such scenarios, the whole transaction would fail.

• In PartyGovernance.findVotingPowerSnapshotIndex, a binary search is performed for the (common) case that snaps[snaps.length - 1].timestamp <= timestamp, i.e. when the timestamp is more recent than the latest checkpoint. Most other implementations (e.g, Compound) explicitly check this before performing a binary search, because it saves gas in expectation.
• In Crowdfund._hashFixedGovernanceOpts, mload(opts) is executed multiple times (https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L331) instead of using the cached result.
• In the for loops, the loop iteration can be marked as unchecked because an overflow is not possible (as the iterator is bounded):

contracts/party/PartyGovernance.sol:306:        for (uint256 i=0; i < opts.hosts.length; ++i) {
contracts/distribution/TokenDistributor.sol:230:        for (uint256 i = 0; i < infos.length; ++i) {
contracts/distribution/TokenDistributor.sol:239:        for (uint256 i = 0; i < infos.length; ++i) {
contracts/crowdfund/Crowdfund.sol:180:        for (uint256 i = 0; i < contributors.length; ++i) {
contracts/crowdfund/Crowdfund.sol:242:        for (uint256 i = 0; i < numContributions; ++i) {
contracts/crowdfund/Crowdfund.sol:300:        for (uint256 i = 0; i < preciousTokens.length; ++i) {
contracts/crowdfund/Crowdfund.sol:348:            for (uint256 i = 0; i < numContributions; ++i) {
contracts/proposals/LibProposal.sol:14:        for (uint256 i = 0; i < preciousTokens.length; ++i) {
contracts/proposals/LibProposal.sol:32:        for (uint256 i = 0; i < preciousTokens.length; ++i) {
contracts/proposals/ArbitraryCallsProposal.sol:52:            for (uint256 i = 0; i < hadPreciouses.length; ++i) {
contracts/proposals/ArbitraryCallsProposal.sol:61:        for (uint256 i = 0; i < calls.length; ++i) {
contracts/proposals/ArbitraryCallsProposal.sol:78:            for (uint256 i = 0; i < hadPreciouses.length; ++i) {
contracts/proposals/ListOnOpenseaProposal.sol:291:            for (uint256 i = 0; i < fees.length; ++i) {