PartyDAO contest - ChristianKuri's results

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfundBase.sol#L73 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/AuctionCrowdfund.sol#L118 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/CollectionBuyCrowdfund.sol#L91 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/Crowdfund.sol#L448-L455 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfundBase.sol#L150-L164

Vulnerability details

Impact

Users cannot withdraw their ETH before the crowdfund ends, even if they do not want to participate in it anymore. If for some reason a user considers that the NFT that the crowdfund is buying is not worth it anymore or they think they wont reach the goal, they cannot withdraw their participation, forcing them to participate in a NFT that they no longer want for a lower particitipation than what they wanted or making the crowdfund lock their ETH forever because the duration can be as high as 34,000 years.

Proof of Concept

Bob wants to buy an ERC721 token listed for 100ETH (Which is a good deal) and creates a crowdfund with the maximum duration which is equal to 34,000 years (it does not need to be that long, 10 years is also a lot of time). 10 users participates with 10ETH each expecting to have 10% of the participation, so the crowdfund now has 100ETH. But now the token is listed for 1,000 ETH. Other 40 users participate with 10ETH each, so the crowdfund now has 500ETH.

Now the token is listed for 10,000 ETH. The users think that the token is not worth it 10,000 ETH and they want to withdraw their ETH, but they cant. And because the crowdfund has a long duration of 34,000 years. They are now stuck with their ETH in the crowdfund for 34,000 years.

In the scenario that the NFT is bought for 10,000 ETH because the crowdfund somehow was able to get funded with that amount, now the users participation is way lower than what they expected, some users participated with 10ETH and expected to have 10% of the participation (Because the token was listed for 100ETH when they participated), now they only own 0.1% and they were not able to reject the buy.

The problem is not only the duration but forcing someone to buy a token for a price that they did not want to buy anymore for a different participation that was originally intended. If someone participates expecting to get 10% of the participation, they should get that participation or be able to withdraw.

crowdfund/BuyCrowdfundBase.sol#L73

crowdfund/AuctionCrowdfund.sol#L118

crowdfund/CollectionBuyCrowdfund.sol#L91

The burn function can only be ran if the crowdfund won or lost (means that the crowdfund expired).

crowdfund/Crowdfund.sol#L448-L455

File: crowdfund/Crowdfund.sol#L448-L455

      function _burn(address payable contributor, CrowdfundLifecycle lc, Party party_)
          private
      {
          // If the CF has won, a party must have been created prior.
          if (lc == CrowdfundLifecycle.Won) {
              if (party_ == Party(payable(0))) {
                  revert NoPartyError();
              }
          } else if (lc != CrowdfundLifecycle.Lost) {
              // Otherwise it must have lost.
              revert WrongLifecycleError(lc);
          }
          ...
      }

crowdfund/BuyCrowdfundBase.sol#L150-L164

File: crowdfund/BuyCrowdfundBase.sol#L150-L164

      function getCrowdfundLifecycle() public override view returns (CrowdfundLifecycle) {
          // If there is a settled price then we tried to buy the NFT.
          if (settledPrice != 0) {
              return address(party) != address(0)
                  // If we have a party, then we succeeded buying the NFT.
                  ? CrowdfundLifecycle.Won
                  // Otherwise we're in the middle of the buy().
                  : CrowdfundLifecycle.Busy;
          }
          if (block.timestamp >= expiry) {
              // Expired but nothing to do so skip straight to lost.
              return CrowdfundLifecycle.Lost;
          }
          return CrowdfundLifecycle.Active;
      }

Tools Used

VSCode & Manual Analysis

Recommended Mitigation Steps

Even tho a duration suggestion could be done on the front end, the idea here is to make the contract secure.

Solutions:

• Lower the maximum duration to a year. (There is no point of allowing such big duration of 34,000 years)
• Allow users to withdraw their ETH before the crowdfund ends.
• Allow the crowdfund to be canceled by the owner and allow everyone to withdraw their funds when that happens.
• Allow the crowdfund participants to change the token id by voting

[GO01] Don't Initialize Variables with Default Value

Impact

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecessary gas.

// from
uint i = 0;
bool b = false;

// to
uint i;
bool b;

Findings:

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/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/party/PartyGovernance.sol::306 => for (uint256 i=0; i < opts.hosts.length; ++i) {
contracts/party/PartyGovernance.sol::432 => uint256 low = 0;
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/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/ListOnOpenseaProposal.sol::291 => for (uint256 i = 0; i < fees.length; ++i) {

[GO02] Cache Array Length Outside of Loop

Impact

Cache the length of arrays in loops (~6 gas per iteration) The overheads outlined below are PER LOOP, excluding the first loop - storage arrays incur a Gwarmaccess (100 gas) - memory arrays use MLOAD (3 gas) - calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

// from
for(uint i; i < someArray.length; ++i){

// to
uint256 length = someArray.length;
for(uint i; i < length; ++i){

Findings:

contracts/crowdfund/CollectionBuyCrowdfund.sol::62 => for (uint256 i; i < hosts.length; i++) {
contracts/crowdfund/Crowdfund.sol::180 => for (uint256 i = 0; i < contributors.length; ++i) {
contracts/crowdfund/Crowdfund.sol::300 => for (uint256 i = 0; i < preciousTokens.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/party/PartyGovernance.sol::306 => for (uint256 i=0; i < opts.hosts.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/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/ListOnOpenseaProposal.sol::291 => for (uint256 i = 0; i < fees.length; ++i) {

[GO03] Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0.

// from
 (amount > 0);

// to
(amount != 0);

Findings:

contracts/crowdfund/Crowdfund.sol::144 => if (initialBalance > 0) {
contracts/crowdfund/Crowdfund.sol::471 => if (votingPower > 0) {