PartyDAO contest - ch0bu's results

LOW

1. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L271) in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/Crowdfund.sol

439			_mint(contributor);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernanceNFT.sol

132			_mint(owner, tokenId);

NON-CRITICAL

1. Floading Pragma version

In the contracts, floating pragmas should not be used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Proof of Concept https://swcregistry.io/docs/SWC-103

All contracts

2. Use a more recent version of Solidity

• Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
• Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

All contracts

3. NatSpec is incomplete

All contracts are missing @author, @title messages. Some contracts do not have any NatSpec messages at all.

4. Event is missing indexedfields

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

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/FractionalizeProposal.sol

22	event FractionalV1VaultCreated(
23		IERC721 indexed token
24		uint256 indexed tokenId,
25		uint256 vaultId,
26		IERC20 vault,
27		uint256 listPrice
28		);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnZoraProposal.sol

45		event ZoraAuctionCreated(
46			uint256 auctionId,
47			IERC721 token,
48			uint256 tokenId,
49			uint256 startingPrice,
50			uint40 duration,
51			uint40 timeoutTime
52		);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ArbitraryCallsProposal.sol

35		event ArbitraryCallExecuted(uint256  proposalId, uint256  idx, uint256  count);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfundBase.sol

52		event Won(Party party, IERC721  token, uint256  tokenId, uint256  settledPrice);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnOpenseaProposal.sol

63		event OpenseaOrderListed(
64			IOpenseaExchange.OrderParameters orderParams,
65			bytes32 orderHash,
66			IERC721 token,
67			uint256 tokenId,
68			uint256 listPrice,
69			uint256 expiry
70		);
71		event OpenseaOrderSold(
72			bytes32 orderHash,
73			IERC721 token,
74			uint256 tokenId,
75			uint256 listPrice
76		);
77		event OpenseaOrderExpired(
78			bytes32 orderHash,
79			IERC721 token,
80			uint256 tokenId,
81			uint256 expiry
82		);
83		// Coordinated event w/OS team to track on-chain orders.
84		event OrderValidated(
85			bytes32 orderHash,
86			address indexed offerer,
87			address indexed zone,
88			IOpenseaExchange.OfferItem[] offer,
89			IOpenseaExchange.ConsiderationItem[] consideration,
90			IOpenseaExchange.OrderType orderType,
91			uint256 startTime,
92			uint256 endTime,
93			bytes32 zoneHash,
94			uint256 salt,
95			bytes32 conduitKey,
96			uint256 counter
97		);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol

151		event Proposed(
152			uint256 proposalId,
153			address proposer,
154			Proposal proposal
155		);
156		event ProposalAccepted(
157			uint256 proposalId,
158			address voter,
159			uint256 weight
160		);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/tokens/IERC1155.sol

20		event ApprovalForAll(address  indexed  owner, address  indexed  operator, bool  approved);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/distribution/ITokenDistributor.sol

37		event DistributionFeeClaimed(
38			ITokenDistributorParty indexed party,
39			address indexed feeRecipient,
40			TokenType tokenType,
41			address token,
42			uint256 amount
43		);

5. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernanceNFT.sol

143		super.transferFrom(owner, to, tokenId);

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/Crowdfund.sol

301		preciousTokens[i].transferFrom(address(this), address(party_), preciousTokenIds[i]);

6. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ProposalStorage.sol

19		uint256  private constant SHARED_STORAGE_SLOT =  uint256(keccak256("ProposalStorage.SharedProposalStorage"));

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ProposalExecutionEngine.sol

80		uint256  private constant _STORAGE_SLOT =  uint256(keccak256('ProposalExecutionEngine.Storage'));

1. Use a more recent version of solidity

• Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
• Use a solidity version of at least 0.8.2 to get compiler automatic inlining
• Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
• 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

All contracts

2. ++i costs less gas compared to i++ or i += 1, same for --i/i--. Especially in for loops

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i.

uint i = 1;  
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

I suggest using ++i instead of i++ to increment the value of an uint variable.

If done inside for loop, saves 6 gas per loop.

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/CollectionBuyCrowdfund.sol

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

3. 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.

As an example: for (uint256 i = 0; i < hadPreciouses.length; ++i) { should be replaced with for (uint256 i; i < hadPreciouses.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/distribution/TokenDistributor.sol

230		for (uint256 i =  0; i < infos.length; ++i) {
239		for (uint256 i =  0; i < infos.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnOpenseaProposal.sol

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

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/LibProposal.sol

14		for (uint256 i =  0; i < preciousTokens.length; ++i) {
32		for (uint256 i =  0; i < preciousTokens.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol

306		for (uint256 i=0; i < opts.hosts.length; ++i) {
432		uint256 low =  0;

4. Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline. This saves 30-40 gas PER LOOP.

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/distribution/TokenDistributor.sol

230		for (uint256 i =  0; i < infos.length; ++i) {
239		for (uint256 i =  0; i < infos.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol

306		for (uint256 i=0; i < opts.hosts.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnOpenseaProposal.sol

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

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/LibProposal.sol

14		for (uint256 i =  0; i < preciousTokens.length; ++i) {
32		for (uint256 i =  0; i < preciousTokens.length; ++i) {

5. <array>.length should not be looked up in every loop of a for-loop

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

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/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) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/distribution/TokenDistributor.sol

230		for (uint256 i =  0; i < infos.length; ++i) {
239		for (uint256 i =  0; i < infos.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol

306		for (uint256 i=0; i < opts.hosts.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnOpenseaProposal.sol

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

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/Crowdfund.sol

180		for (uint256 i =  0; i < contributors.length; ++i) {
300		for (uint256 i =  0; i < preciousTokens.length; ++i) {

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/LibProposal.sol

14		for (uint256 i =  0; i < preciousTokens.length; ++i) {
32		for (uint256 i =  0; i < preciousTokens.length; ++i) {

6. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

These can be found in most contracts that have variable assignments. Whenever possible consider using uint256 instead of smaller uint types.

Here is just a couple of examples: https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfund.sol#L30 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfund.sol#L39 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/CollectionBuyCrowdfund.sol#L51 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/BuyCrowdfundBase.sol#L94

7. abi.encode() is less efficient than abi.encodepacked()

Whenever possible consider using abi.encodepacked() to save gas.

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnZoraProposal.sol

115		return  abi.encode(ZoraStep.ListedOnZora, ZoraProgressData({

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnOpenseaProposal.sol

164		return  abi.encode(ListOnOpenseaStep.ListedOnZora, ZoraProgressData({
219		return  abi.encode(ListOnOpenseaStep.ListedOnOpenSea, orderHash, expiry);

8. Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

Use uint256(1) and uint256(2) for true/false

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/globals/Globals.sol

12		mapping(uint256  =>  mapping(bytes32  =>  bool)) private _includedWordValues;

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/distribution/TokenDistributor.sol

30		mapping(uint256  =>  bool) hasPartyTokenClaimed;
62		bool  public emergencyActionsDisabled;

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/Crowdfund.sol

106		bool  private _splitRecipientHasBurned;

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol

148		mapping (address  =>  bool) hasVoted;
197		bool  public emergencyExecuteDisabled;
207		mapping(address  =>  bool) public isHost;

9. Not using the named return variables when a function returns, wastes deployment gas

This can be found in most contracts that have return functions.

Some examples: https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/globals/Globals.sol#L32 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/globals/Globals.sol#L36 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/globals/Globals.sol#L40 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/crowdfund/CrowdfundNFT.sol#L99 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernanceNFT.sol#L72 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnZoraProposal.sol#L181 https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ListOnZoraProposal.sol#L193