PartyDAO contest - delfin454000's results

Typos

---

BuyCrowdfundBase.sol: L31

        // An address that receieves an extra share of the final voting power

Change receieves to receives

---

ListOnOpenseaProposal.sol: L303

        // Emit the the coordinated OS event so their backend can detect this order.

Remove repeated word the

---

The same typo (recipeint) occurs in both lines below:

Crowdfund.sol: L46

PartyGovernance.sol: L86

Example (Crowdfund.sol: L46):

        // Fee recipeint for governance distributions.

Change recipeint to recipient

---

Crowdfund.sol: L258

    // Deploys and initializes a a `Party` instance via the `PartyFactory`

Remove repeated word a

---

---

Inconsistent wrap of long comments

Long comments are wrapped using inconsistent syntax in the contest. Some are wrapped conventionally, using a few additional spaces after the /// or // in the second (and any subsequent lines), as shown below:

PartyGovernance.sol: L713-714

    /// @dev proposal.cancelDelay seconds must have passed since it was first
    ///       executed for this to be valid.

This flags for the reader that the comment is continuing. However, the majority of multiline comments in the contest are not indented in the second (and any subsequent wrapped lines). For example:

TokenDistributor.sol: L67-68

    /// Allows one to simply transfer and call `createDistribution()` without
    /// fussing with allowances.

In addition, some comments wrap using extra-wide indentations, as shown below:

CrowdfundFactory.sol: L55-60

    /// @notice Create a new crowdfund to bid on an auction for a specific NFT
    ///         (i.e. with a known token ID).
    /// @param opts Options used to initialize the crowdfund. These are fixed
    ///             and cannot be changed later.
    /// @param createGateCallData Encoded calldata used by `createGate()` to create
    ///                           the crowdfund if one is specified in `opts`.

Suggestion: For increased readability, wrap multiline comments using consistent syntax throughout Party DAO

---

---

Named return variable not used

The existence of a named return variable that is not subsequently used (here, newGateKeeperId) is potentially perplexing

---

CrowdfundFactory.sol: L107-122

    function _prepareGate(
        IGateKeeper gateKeeper,
        bytes12 gateKeeperId,
        bytes memory createGateCallData
    )
        private
        returns (bytes12 newGateKeeperId)
    {
        if (
            address(gateKeeper) == address(0) ||
            gateKeeperId != bytes12(0)
        ) {
            // Using an existing gate on the gatekeeper
            // or not using a gate at all.
            return gateKeeperId;
        }

---

---

Missing require revert strings

A require message should be included to enable users to understand reason for failure

Recommendation: Add a brief (<= 32 char) explanatory message to each of the three requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).

---

ReadOnlyDelegateCall.sol: L20

        require(msg.sender == address(this));

---

ProposalExecutionEngine.sol: L246

        require(proposalType != ProposalType.Invalid);

---

ProposalExecutionEngine.sol: L247

        require(uint8(proposalType) < uint8(ProposalType.NumProposalTypes));

---

---

Use consistent initialization of counters in for loops

One of the for loop counters is not initiated to zero, as shown below:

CollectionBuyCrowdfund.sol: L62

        for (uint256 i; i < hosts.length; i++) {

However, the remaining 13 of the for loop counters are initialized to zero, as the example below shows:

ArbitraryCallsProposal.sol: L52

            for (uint256 i = 0; i < hadPreciouses.length; ++i) {

It is not necessary to initialize for loop counters to zero since this is their default value. For consistency, it makes sense to omit counter initializations in all of the for loops

---

---

State variables should not be initialized to their default values

Initializing uint variables to their default value of 0 is unnecessary and costs gas

---

PartyGovernance.sol: L432

        uint256 low = 0;

Change to:

        uint256 low;

---

---

Array length should not be looked up in every iteration of a for loop

Since calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop

---

CollectionBuyCrowdfund.sol: L62-67

        for (uint256 i; i < hosts.length; i++) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;
            }
        }

Suggestion:

        uint256 numHosts = hosts.length; 
        for (uint256 i; i < numHosts; i++) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;
            }
        }

---

Similarly for the eleven additional for loops referenced below:

ArbitraryCallsProposal.sol: L52-57

ArbitraryCallsProposal.sol: L61-74

ArbitraryCallsProposal.sol: L78-87

TokenDistributor.sol: L230-232

TokenDistributor.sol: L239-241

ListOnOpenseaProposal.sol: L291-298

Crowdfund.sol: L180-182

Crowdfund.sol: L300-302

PartyGovernance.sol: L306-308

LibProposal.sol: L14-18

LibProposal.sol: L32-36

---

---

Use ++i instead of i++ to increase count in a for loop

Since use of i++ (or equivalent counter) costs more gas, it is better to use ++i

---

CollectionBuyCrowdfund.sol: L62-67

        for (uint256 i; i < hosts.length; i++) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;
            }
        }

Suggestion:

        uint256 numHosts = hosts.length; 
        for (uint256 i; i < numHosts; ++i) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;
            }
        }

___
___


### Avoid use of default "checked" behavior in a `for` loop
Underflow/overflow checks are made every time `++i` (or equivalent counter) is called. Such checks are unnecessary since `i` is already limited. Therefore, use `unchecked ++i` instead
___
[CollectionBuyCrowdfund.sol: L62-67](https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CollectionBuyCrowdfund.sol#L62-L67)
```solidity
        for (uint256 i; i < hosts.length; i++) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;
            }
        }

Suggestion:

        uint256 numHosts = hosts.length; 
        for (uint256 i; i < numHosts;) {
            if (hosts[i] == msg.sender) {
                isHost = true;
                break;

                unchecked {
                  ++i;
              }
            }
        }

---

Similarly for the thirteen additional for loops referenced below:

ArbitraryCallsProposal.sol: L52-57

ArbitraryCallsProposal.sol: L61-74

ArbitraryCallsProposal.sol: L78-87

TokenDistributor.sol: L230-232

TokenDistributor.sol: L239-241

ListOnOpenseaProposal.sol: L291-298

Crowdfund.sol: L180-182

Crowdfund.sol: L241-244

Crowdfund.sol: L300-302

Crowdfund.sol: L347-362

PartyGovernance.sol: L306-308

LibProposal.sol: L14-18

LibProposal.sol: L32-36

---

---