PartyDAO contest - 0x5rings's results

Findings: require()/revert() strings longer than 32 bytes cost extra gas

Code: contracts/KoansMarketWrapper.sol - line 67, line 92 Contracts/NounsMarketWrapper.sol - line 65, line 90

Explanation:

---

Findings: REQUIRE() SHOULD BE USED INSTEAD OF ASSERT()

Code: contracts/CrowdfundNFT.sol - line 166 Contracts/TokenDistributor.sol - line 385, line 411 Contracts/ListOnOpenseaProposal.sol - line 221

Explanation:

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction’s available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that “The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix”.

Mitigation: Use of Custom Errors or require

---

Findings: _SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE

Code: contracts/Crowdfund.sol - line 451 Contracts/PartyGovernance.sol - line 132

Explanation: _mint() is discouraged in favour of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

---

Findings: FUNCTION MISSING COMMENTS

Code: Some functions are missing comments. Can lead to confusion for future devs. / readability.

Explanation: contracts/Crowdfund.sol - line 377, line 443, line 490 Contracts/CrowdfundFactory.sol - line 106 Contracts/crowdfundNFT.sol - line 136, line 149, line 159

(More instances than can count)

---

Findings: Use of Block.timestamp

Code: contract/crowdfund/BuyCrowdfundBase.sol - line 73, line 159

Explanation: Block timestamps should not be used to be the deciding factor whether the _expiry ends. Miners have the ability to adjust timestamps, rearrange tx orders. Which can be dangerous / allows a user to game the system and _Crowdfund.

Mitigation: Use of Block.number, whereby miners are unable to easily manipulate the block number.

---

Findings: UNUSED/EMPTY RECEIVE()/FALLBACK() FUNCTION

Code: AuctionCrowdfund.sol - line 144,

Explanation: Empty blocks should be removed or emit something / fallback or receive function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))

---

Findings: USE OF FLOATING PRAGMA Code: contracts * - (contract versions)

Explanation: 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.

---

Findings: ++I COSTS LESS GAS COMPARED TO I++ OR I += 1 (SAME FOR --I VS I-- OR I -= 1)

Code: contracts/CollectionBuyCrowfund.sol - line 632 Contracts/Crowdfund.sol - line 180

Explanation:

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

i += 1 is the most expensive form i++ costs 6 gas less than i += 1 ++i costs 5 gas less than i++ (11 gas less than i += 1)

---

Findings: AN ARRAY’S LENGTH SHOULD BE CACHED TO SAVE GAS IN FOR-LOOPS

Code: contracts/CollectionBuyCrowfund.sol - line 62 Contracts/Crowdfund.sol - line 180, line 300 Contracts/TokenDistributor.sol - 230 Contracts/PartyGovernance.sol - 306

Explanation:

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

---

Findings: It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Code: Contracts/Crowdfund.sol - line 247, line 307, line 356 Contracts/TokenDistributor.sol - line 230

Explanation:

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

---

Findings: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES (or is not a state variable)

Code: contracts/Crowdfund.sol - line 427

Explanation:

---

Findings: MULTIPLICATION/DIVISION BY TWO SHOULD USE BIT SHIFTING

Code: contracts/PartyGovernance.sol - line 434

Explanation: <x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas