PartyDAO contest - __141345__'s results

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

crowdfund/BuyCrowdfundBase.sol
52:     event Won(Party party, IERC721 token, uint256 tokenId, uint256 settledPrice);

crowdfund/Crowdfund.sol
82:    event Burned(address contributor, uint256 ethUsed, uint256 ethOwed, uint256 votingPower);
83:    event Contributed(address contributor, uint256 amount, address delegate, uint256 previousTotalContributions);

distribution/ITokenDistributor.sol
37-43:
    event DistributionFeeClaimed(
        ITokenDistributorParty indexed party,
        address indexed feeRecipient,
        TokenType tokenType,
        address token,
        uint256 amount
    );

party/PartyGovernance.sol
151-162:
    event Proposed(
        uint256 proposalId,
        address proposer,
        Proposal proposal
    );
    event ProposalAccepted(
        uint256 proposalId,
        address voter,
        uint256 weight
    );

    event PartyInitialized(GovernanceOpts opts, IERC721[] preciousTokens, uint256[] preciousTokenIds);

167:    event DistributionCreated(ITokenDistributor.TokenType tokenType, address token, uint256 tokenId);

proposals/ArbitraryCallsProposal.sol
35:     event ArbitraryCallExecuted(uint256 proposalId, uint256 idx, uint256 count);

proposals/FractionalizeProposal.sol
22-28:
    event FractionalV1VaultCreated(
        IERC721 indexed token,
        uint256 indexed tokenId,
        uint256 vaultId,
        IERC20 vault,
        uint256 listPrice
    );

proposals/ListOnOpenseaProposal.sol
63-97:
    event OpenseaOrderListed(
        IOpenseaExchange.OrderParameters orderParams,
        bytes32 orderHash,
        IERC721 token,
        uint256 tokenId,
        uint256 listPrice,
        uint256 expiry
    );
    event OpenseaOrderSold(
        bytes32 orderHash,
        IERC721 token,
        uint256 tokenId,
        uint256 listPrice
    );
    event OpenseaOrderExpired(
        bytes32 orderHash,
        IERC721 token,
        uint256 tokenId,
        uint256 expiry
    );
    // Coordinated event w/OS team to track on-chain orders.
    event OrderValidated(
        bytes32 orderHash,
        address indexed offerer,
        address indexed zone,
        IOpenseaExchange.OfferItem[] offer,
        IOpenseaExchange.ConsiderationItem[] consideration,
        IOpenseaExchange.OrderType orderType,
        uint256 startTime,
        uint256 endTime,
        bytes32 zoneHash,
        uint256 salt,
        bytes32 conduitKey,
        uint256 counter
    );

proposals/ListOnZoraProposal.sol
45-52:
    event ZoraAuctionCreated(
        uint256 auctionId,
        IERC721 token,
        uint256 tokenId,
        uint256 startingPrice,
        uint40 duration,
        uint40 timeoutTime
    );

AVOID FLOATING PRAGMAS: THE VERSION SHOULD BE LOCKED

The pragma declared across the solution is ^0.8. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

crowdfund/CollectionBuyCrowdfund.sol
62:     for (uint256 i; i < hosts.length; i++) {

crowdfund/Crowdfund.sol
180:    for (uint256 i = 0; i < contributors.length; ++i) {
300:    for (uint256 i = 0; i < preciousTokens.length; ++i) {

distribution/TokenDistributor.sol
230:    for (uint256 i = 0; i < infos.length; ++i) {
239:    for (uint256 i = 0; i < infos.length; ++i) {

party/PartyGovernance.sol
306:    for (uint256 i=0; i < opts.hosts.length; ++i) {

proposals/ArbitraryCallsProposal.sol
52:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {
61:     for (uint256 i = 0; i < calls.length; ++i) {
78:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {

proposals/LibProposal.sol
14:     for (uint256 i = 0; i < preciousTokens.length; ++i) {
32:     for (uint256 i = 0; i < preciousTokens.length; ++i) {

proposals/ListOnOpenseaProposal.sol
291:    for (uint256 i = 0; i < fees.length; ++i) {

Suggestion: Cache the length before the loop.

    uint length = names.length;

FOR-LOOP unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

crowdfund/CollectionBuyCrowdfund.sol
62:     for (uint256 i; i < hosts.length; i++) {

crowdfund/Crowdfund.sol
180:    for (uint256 i = 0; i < contributors.length; ++i) {
242:    for (uint256 i = 0; i < numContributions; ++i) {
300:    for (uint256 i = 0; i < preciousTokens.length; ++i) {
348:    for (uint256 i = 0; i < numContributions; ++i) {

distribution/TokenDistributor.sol
230:    for (uint256 i = 0; i < infos.length; ++i) {
239:    for (uint256 i = 0; i < infos.length; ++i) {

party/PartyGovernance.sol
306:    for (uint256 i=0; i < opts.hosts.length; ++i) {

proposals/ArbitraryCallsProposal.sol
52:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {
61:     for (uint256 i = 0; i < calls.length; ++i) {
78:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {

proposals/LibProposal.sol
14:     for (uint256 i = 0; i < preciousTokens.length; ++i) {
32:     for (uint256 i = 0; i < preciousTokens.length; ++i) {

proposals/ListOnOpenseaProposal.sol
291:    for (uint256 i = 0; i < fees.length; ++i) {

Suggestion: For readability, uncheck the whole for loop is the same.

    unchecked{
            for (uint256 i; i < length; ++i) {
            }
    }

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

crowdfund/Crowdfund.sol
180:    for (uint256 i = 0; i < contributors.length; ++i) {
242:    for (uint256 i = 0; i < numContributions; ++i) {
300:    for (uint256 i = 0; i < preciousTokens.length; ++i) {
348:    for (uint256 i = 0; i < numContributions; ++i) {

distribution/TokenDistributor.sol
230:    for (uint256 i = 0; i < infos.length; ++i) {
239:    for (uint256 i = 0; i < infos.length; ++i) {

party/PartyGovernance.sol
306:    for (uint256 i=0; i < opts.hosts.length; ++i) {

proposals/ArbitraryCallsProposal.sol
52:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {
61:     for (uint256 i = 0; i < calls.length; ++i) {
78:     for (uint256 i = 0; i < hadPreciouses.length; ++i) {

proposals/LibProposal.sol
14:     for (uint256 i = 0; i < preciousTokens.length; ++i) {
32:     for (uint256 i = 0; i < preciousTokens.length; ++i) {

proposals/ListOnOpenseaProposal.sol
291:    for (uint256 i = 0; i < fees.length; ++i) {

Suggestion: For readability, uncheck the whole for loop is the same.

The demo of the loop gas comparison can be seen here.

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

crowdfund/Crowdfund.sol
243:    ethContributed += contributions[i].amount;
352:    ethOwed += c.amount;
355:    ethUsed += c.amount;
359:    ethUsed += partialEthUsed;
374:    votingPower += (splitBps_ * totalEthUsed + (1e4 - 1)) / 1e4; // round up
411:    totalContributions += amount;
427:    lastContribution.amount += amount;

party/PartyGovernance.sol
595:    values.votes += votingPower;

959:    newDelegateDelegatedVotingPower -= oldSnap.intrinsicVotingPower;

distribution/TokenDistributor.sol
381:    _storedBalances[balanceId] -= amount;

proposals/ArbitraryCallsProposal.sol
72:     ethAvailable -= calls[i].value;

!= 0 uses less gas than > 0

!= 0 costs less gas compared to > 0 for unsigned integers with the optimizer enabled (6 gas). Here is the code as a demo. Opcode discussion.

crowdfund/Crowdfund.sol
471:    if (votingPower > 0) {

Suggestion:

• Changing > 0 with != 0.
• Enable the Optimizer.

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

distribution/TokenDistributor.sol
63-71:
    /// @notice Last distribution ID for a party.
    mapping(ITokenDistributorParty => uint256) public lastDistributionIdPerParty;

    // tokenDistributorParty => distributionId => DistributionState
    mapping(ITokenDistributorParty => mapping(uint256 => DistributionState)) private _distributionStates;

globals/Globals.sol
9-12:
    // key -> word value
    mapping(uint256 => bytes32) private _wordValues;
    // key -> word value -> isIncluded
    mapping(uint256 => mapping(bytes32 => bool)) private _includedWordValues;

party/PartyGovernance.sol
207-215:
    mapping(address => bool) public isHost;
    /// @notice The last person a voter delegated its voting power to.
    mapping(address => address) public delegationsByVoter;
    // Constant governance parameters, fixed from the inception of this party.
    GovernanceValues private _governanceValues;

    // Snapshots of voting power per user, each sorted by increasing time.
    mapping(address => VotingPowerSnapshot[]) private _votingPowerSnapshotsByVoter;

EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO keccak256(),` SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Constant expressions are left as expressions, not constants.

When a constant declared as:

proposals/ProposalExecutionEngine.sol
80:     uint256 private constant _STORAGE_SLOT = uint256(keccak256('ProposalExecutionEngine.Storage'));

proposals/ProposalStorage.sol
19:     uint256 private constant SHARED_STORAGE_SLOT = uint256(keccak256("ProposalStorage.SharedProposalStorage"));

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

consequences:

• each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)
• since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

reference: https://github.com/ethereum/solidity/issues/9232

Variable re-arrangement by storage packing

Reference: Layout of State Variables in Storage.

1. crowdfund/AuctionCrowdfund.sol crowdfund/BuyCrowdfund.sol crowdfund/BuyCrowdfundBase.sol crowdfund/CollectionBuyCrowdfund.sol

An address occupies 20 bytes, another 12 bytes can be packed together into the same slot. In struct AuctionCrowdfundOptions, uint40 duration and bytes12 gateKeeperId can be placed next to an address variable. Originally, the struct IGateKeeper and FixedGovernanceOpts each occupy 1 slot, the other uint and address occupy total 5 slots. After re-arrangement, uint and address will occupy total 3 slots, 2 slots storage in total can be saved.

        // How long this crowdfund has to bid on the NFT, in seconds.
        uint40 duration;
        // Maximum bid allowed.
        uint96 maximumBid;
        // An address that receives a portion of the final voting power
        // when the party transitions into governance.
        address payable splitRecipient;
        // What percentage (in bps) of the final total voting power `splitRecipient`
        // receives.
        uint16 splitBps;
        // If ETH is attached during deployment, it will be interpreted
        // as a contribution. This is who gets credit for that contribution.
        address initialContributor;
        // If there is an initial contribution, this is who they will delegate their
        // voting power to when the crowdfund transitions to governance.
        address initialDelegate;
        // The gatekeeper contract to use (if non-null) to restrict who can
        // contribute to this crowdfund.
        IGateKeeper gateKeeper;
        // The gate ID within the gateKeeper contract to use.
        bytes12 gateKeeperId;

can be rearranged as (see the tag with // ~rearrange):

crowdfund/AuctionCrowdfund.sol

        // slot 1 uint96 + address (12 + 20 = 32) bytes
        // Maximum bid allowed.
        uint96 maximumBid;
        // An address that receives a portion of the final voting power
        // when the party transitions into governance.
        address payable splitRecipient;

        // slot 2 uint40 + uint16 + address (5 + 2 + 20 = 27) bytes
        // ~rearrange
        // How long this crowdfund has to bid on the NFT, in seconds.
        uint40 duration;        
        // What percentage (in bps) of the final total voting power `splitRecipient`
        // receives.
        uint16 splitBps;
        // If ETH is attached during deployment, it will be interpreted
        // as a contribution. This is who gets credit for that contribution.
        address initialContributor;

        // slot 3 bytes12 + address (12 + 20 = 32) bytes
        // ~rearrange
        // The gate ID within the gateKeeper contract to use.
        bytes12 gateKeeperId;
        // If there is an initial contribution, this is who they will delegate their
        // voting power to when the crowdfund transitions to governance.
        address initialDelegate;

        // The gatekeeper contract to use (if non-null) to restrict who can
        // contribute to this crowdfund.
        IGateKeeper gateKeeper;

2. crowdfund/Crowdfund.sol

uint96 public totalContributions can be moved before uint16 public splitBps, 1 slot storage can be saved.

    /// @notice The total (recorded) ETH contributed to this crowdfund.
    uint96 public totalContributions;
    /// @notice The gatekeeper contract to use (if non-null) to restrict who can
    ///         contribute to the party.
    IGateKeeper public gateKeeper;
    /// @notice The ID of the gatekeeper strategy to use.
    bytes12 public gateKeeperId;
    /// @notice Who will receive a reserved portion of governance power when
    ///         the governance party is created.
    address payable public splitRecipient;
    /// @notice How much governance power to reserve for `splitRecipient`,
    ///         in bps, where 10,000 = 100%.
    uint16 public splitBps;
    // Whether the share for split recipient has been claimed through `burn()`.
    bool private _splitRecipientHasBurned;

can be rearranged as

    /// @notice The gatekeeper contract to use (if non-null) to restrict who can
    ///         contribute to the party.
    IGateKeeper public gateKeeper;

    /// @notice The ID of the gatekeeper strategy to use.
    bytes12 public gateKeeperId;
    /// @notice Who will receive a reserved portion of governance power when
    ///         the governance party is created.
    address payable public splitRecipient;

    // ~rearrange
    /// @notice The total (recorded) ETH contributed to this crowdfund.
    uint96 public totalContributions;
    /// @notice How much governance power to reserve for `splitRecipient`,
    ///         in bps, where 10,000 = 100%.
    uint16 public splitBps;
    // Whether the share for split recipient has been claimed through `burn()`.
    bool private _splitRecipientHasBurned;

3. vendor/markets/IZoraAuctionHouse.sol

bool approved can be moved before uint8 curatorFeePercentage, 1 slot storage can be saved.

        // Whether or not the auction curator has approved the auction to start
        bool approved;
        // The current highest bid amount
        uint256 amount;
        // The length of time to run the auction for, after the first bid was made
        uint256 duration;
        // The time of the first bid
        uint256 firstBidTime;
        // The minimum price of the first bid
        uint256 reservePrice;
        // The sale percentage to send to the curator
        uint8 curatorFeePercentage;

can be rearranged as

        // The current highest bid amount
        uint256 amount;
        // The length of time to run the auction for, after the first bid was made
        uint256 duration;
        // The time of the first bid
        uint256 firstBidTime;
        // The minimum price of the first bid
        uint256 reservePrice;

        // ~rearrange
        // Whether or not the auction curator has approved the auction to start
        bool approved;
        // The sale percentage to send to the curator
        uint8 curatorFeePercentage;