PartyDAO contest - Deivitto's results

A protocol for buying, using, and selling NFTs as a group.

General Information

Platform: Code4rena

Start Date: 12/09/2022

Pot Size: $75,000 USDC

Total HM: 19

Participants: 110

Period: 7 days

Judge: HardlyDifficult

Total Solo HM: 9

Id: 160

League: ETH

PartyDAO

Findings Distribution

Researcher Performance

Rank: 27/110

Findings: 2

Award: $161.80

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository PartyDAO

Findings Information

🌟 Selected for report: Lambda

Also found by: 0x1f8b, 0x4non, 0x52, 0x5rings, 0xDanielC, 0xNazgul, 0xSmartContract, 0xbepresent, Anth3m, Aymen0909, B2, CRYP70, CertoraInc, Ch_301, Chom, ChristianKuri, CodingNameKiki, Deivitto, Funen, JC, JansenC, Jeiwan, KIntern_NA, MasterCookie, MiloTruck, Olivierdem, PaludoX0, R2, RaymondFam, ReyAdmirado, StevenL, The_GUILD, Tomo, Trust, V_B, __141345__, asutorufos, ayeslick, bin2chen, brgltd, bulej93, c3phas, cccz, ch0bu, cryptphi, csanuragjain, d3e4, delfin454000, djxploit, erictee, fatherOfBlocks, gogo, hansfriese, indijanc, ladboy233, leosathya, lukris02, malinariy, martin, pedr02b2, pfapostol, rvierdiiev, slowmoses, smiling_heretic, tnevler, wagmi

Awards

91.9689 USDC - $91.97

Labels

bug

low quality report

QA (Quality Assurance)

sponsor acknowledged

old-submission-method

External Links

Link to GitHub issue

Low

Prevent div by 0

Impact

Precautions are not taken to not divide by 0, this is important because this would revert the code.

Especially in the _getTotalVotingPower() that is operating through a saved variable that can actually be 0, if that occurs, it will revert always

Proof of Concept

Navigate to the following contracts,

Input variables are not being checked so they can be 0 and therefore a division by zero can occur https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1079

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1057-L1067

Function can return a 0 value as in the logic behind is not being checked 0 value, so it will be equal to zero therefore div by zero will occur. https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L111-L113

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge cases.

Variable shadows another variable

Summary

Name shadowing where two or more variables/functions share the same name could be confusing to developers and/or reviewers

Details

PartyGovernanceNFT._GLOBALS shadows PartyGovernance._GLOBALS Crowdfund._GLOBALS shadows CrowdfundNFT._GLOBALS ProposalExecutionEngine._GLOBALS shadows ListOnZoraProposal._GLOBALS and ListOnOpenseaProposal._GLOBALS

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L24 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L87 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L77

Mitigation

Replace _GLOBALS variable to another name in these cases for example _GLOBALS_PARTY_GOVERNANCE in the first case

Unused `receive()` function will lock ether in contract

Summary

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/Party.sol#L47 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L144

Mitigation

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.

Missing checks for address(0x0) when assigning values to `address` state or `immutable` variables

Summary

Zero address should be checked for state variables, immutable variables. A zero address can lead into problems.

Github Permalinks

Mitigation

Check zero address before assigning or using it

block.timestamp used as time proxy in a strict equality

Summary

Risk of using block.timestamp for time should be considered.

Details

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times. Strict equalities are suggested to be avoid in general, but in timestamp is pretty important as this can be manipulated (since merge +-1s)

References

SWC ID: 116

Github Permalinks

lastSnap.timestamp == snap.timestamp https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L973-L987

Mitigation

Consider using an oracle for precision
Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.

Use of asserts()

Impact

From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix. With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam. You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):

Details

The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. A reachable assertion can mean one of two things:

A bug exists in the contract that allows it to enter an invalid state; The assert statement is used incorrectly, e.g. to validate inputs.

References

SWC ID: 110

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/ReadOnlyDelegateCall.sol#L30 assert(false);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/FractionalizeProposal.sol#L67 assert(vault.balanceOf(address(this)) == supply);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundNFT.sol#L166 assert(false); // Will not be reached.

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L179 assert(false); // Will not be reached.

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L120 assert(step == ZoraStep.ListedOnZora);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L142 assert(currentInProgressProposalId == 0);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L385 assert(tokenType == TokenType.Erc20);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L411 assert(tokenType == TokenType.Erc20);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L221 assert(step == ListOnOpenseaStep.ListedOnOpenSea);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L302 assert(SEAPORT.validate(orders));

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L504 assert(tokenType == ITokenDistributor.TokenType.Erc20);

Recommended Mitigation Steps

Substitute asserts with require/revert.

Informational

Use of magic numbers is confusing and risky

Summary

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

Details

Values are hardcoded and would be more readable and maintainable if declared as a constant

1e4 and 1e18

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L112 return votingPowerByTokenId[tokenId] * 1e18 / _getTotalVotingPower();

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L261 + (1e18 - 1)

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L263 / 1e18

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L335 if (args.feeBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L352 uint128 fee = supply * args.feeBps / 1e4;

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L129 if (opts.governanceOpts.feeBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L132 if (opts.governanceOpts.passThresholdBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L135 if (opts.splitBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L370 votingPower = ((1e4 - splitBps_) * ethUsed) / 1e4;

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L374 votingPower += (splitBps_ * totalEthUsed + (1e4 - 1)) / 1e4; // round up

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L280 if (opts.feeBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L283 if (opts.passThresholdBps > 1e4) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1062 uint256 acceptanceRatio = (totalVotes * 1e4) / totalVotingPower;

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L1078 return uint256(voteCount) * 1e4

Mitigation

Replace magic hardcoded numbers with declared constants.

Missing indexed event parameters

Summary

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks

Mitigation

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Naming convention of state variable non constant

Summary

Only constants are suggested to use style CONSTANTS_WITH_UNDERSCORES, other variables are suggested to use camelCase

Github Permalinks

these immutable variables assigned here are not constant and so they are not expected to be camelCase https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L119 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L26 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundNFT.sol#L35 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L94 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyFactory.sol#L22 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L267 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L45 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/FractionalizeProposal.sol#L35 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L71-L72 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L94 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/Proxy.sol#L17 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/Implementation.sol#L11

Mitigation

Rename to camelCase

Missing inheritance

PartyHelpers should inherit from ZoraHelpers CrowdfundFactory should inherit from ZoraHelpers Proxy should inherit from ZoraHelpers

Github permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/PartyHelpers.sol#L7-L126 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/Proxy.sol#L8-L37 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L13-L131

Mitigation

Add the inheritance

Different versions of pragma

Summary

Some of the contracts include an unlocked pragma, e.g., pragma solidity >=0.8.4.

Locking the pragma helps ensure that contracts are not accidentally deployed using an old compiler version with unfixed bugs.

Mitigation

Lock pragmas to a specific Solidity version. Consider converting >= 0.8.0 into 0.8.16 Consider converting ^ 0.8.0 into 0.8.16

Missing onlyConstructor modifier on initialize

Following the style of the other initialize functions, this is missing the modifier

Github Permalink

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L117-L126

Mitigation

Ensure that the modifier is applied to the contract.

Missing event on interface

Summary

safeTransferFrom, setApprovalForAll and approve expect a Transfer event to be emit, however, it is missed

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L136-L166

Mitigation

Add the event emission

Maximum line length exceeded

Summary

Long lines should be wrapped to conform with Solidity Style guidelines.

Details

Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalStorage.sol#L10

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalStorage.sol#L19

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/FractionalizeProposal.sol#L10

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/FractionalizeProposal.sol#L30

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L71

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L75

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L79

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L17

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L18

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L94

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L95

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L102

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L31

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L50

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L71

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L73

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L46

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L71

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L236

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L15

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L202

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L203

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L78

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L83

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L162

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L167

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L430

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/tokens/IERC721.sol#L11

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/vendor/FractionalV1.sol#L8

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/vendor/FractionalV1.sol#L27

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/ITokenDistributor.sol#L118

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/vendor/IOpenseaExchange.sol#L129

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/vendor/IOpenseaExchange.sol#L130

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/vendor/IOpenseaExchange.sol#L135

Mitigation

Reduce line length to less than 99 at least to improve maintainability and readability of the code

Missing error messages in require statements

Summary

Require/revert statements should include error messages in order to help at monitoring the system.

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/utils/ReadOnlyDelegateCall.sol#L20 require(msg.sender == address(this));

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L246 require(proposalType != ProposalType.Invalid);

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L247 require(uint8(proposalType) < uint8(ProposalType.NumProposalTypes));

Mitigation

Add error messages

#0 - HardlyDifficult
2022-10-04T09:50:54Z

Merging with https://github.com/code-423n4/2022-09-party-findings/issues/351

Findings Information

🌟 Selected for report: CertoraInc

Also found by: 0x1f8b, 0x4non, 0x5rings, 0x85102, 0xNazgul, 0xSmartContract, 0xkatana, Amithuddar, Aymen0909, B2, Bnke0x0, CRYP70, Chom, ChristianKuri, CodingNameKiki, Deivitto, Diraco, Fitraldys, Funen, IgnacioB, JAGADESH, JC, Lambda, LeoS, Matin, Metatron, MiloTruck, Noah3o6, Ocean_Sky, Olivierdem, PaludoX0, RaymondFam, ReyAdmirado, Rohan16, Rolezn, Saintcode_, Sm4rty, SnowMan, StevenL, Tomio, Tomo, V_B, Waze, __141345__, ajtra, asutorufos, aysha, brgltd, bulej93, c3phas, ch0bu, d3e4, delfin454000, dharma09, djxploit, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, got_targ, ignacio, jag, karanctf, ladboy233, leosathya, lukris02, m_Rassska, malinariy, martin, natzuu, pashov, peanuts, peiw, pfapostol, prasantgupta52, robee, simon135, slowmoses, sryysryy, tnevler

Awards

69.8346 USDC - $69.83

Labels

bug

G (Gas Optimization)

low quality report

sponsor acknowledged

old-submission-method

External Links

Link to GitHub issue

GAS

Constants expressions are expressions, not constants.

Summary

Constant expressions are left as expressions, not constants.

Details

Reference to this kind of issue: https://github.com/ethereum/solidity/issues/9232

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L80 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalStorage.sol#L19

Mitigation

Use immutable for this expressions

Public function visibility can be made external

Summary

Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.

Details

If a function is never called from the contract it should be marked as external. This will save gas.

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L167-L171 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L191-L209 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/tokens/ERC721Receiver.sol#L14-L21

Mitigation

Consider changing visibility from public to external.

use of custom errors rather than revert() / require() error message

Summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

Details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundNFT.sol#L29 revert('ALWAYS FAILING');

Mitigation

replace each error message in a require by a custom error

use != rather than >0 for unsigned integers in require() statements

Summary

When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When Using != , the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L144 if (initialBalance > 0) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L471 if (votingPower > 0) {

Mitigation

Use != 0 rather than > 0 for unsigned integers in require() statements.

Using bools for storage incurs overhead {

Summary

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.

Details

Here is one example of OpenZeppelin about this optimization https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L62 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L106

Mitigation

Consider using uint256 with values 0 and 1 rather than bool

Store using Struct over multiple mappings

Summary

All these variables could be combine in a Struct in order to reduce the gas cost.

Details

As noticed in: https://gist.github.com/alexon1234/b101e3ac51bea3cbd9cf06f80eaa5bc2 When multiple mappings that access the same addresses, uints, etc, all of them can be mixed into an struct and then that data accessed like: mapping(datatype => newStructCreated) newStructMap; Also, you have this post where it explains the benefits of using Structs over mappings https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L10 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L12

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L71 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L64

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L207 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L209 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L215

Mitigation

Consider mixing different mappings into an struct when able in order to save gas.

Pack structs tightly

Summary

Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage.

Details

You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L33-L71 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfund.sol#L19-L54 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L20-L50 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CollectionBuyCrowdfund.sol#L22-L55

Mitigation

Order structs to reduce gas usage.

Explicit initialization

Summary

It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Details

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…).

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L432

Mitigation

Don't initialize variables to default value

Index initialized in for loop

Summary

In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.

Github Permalinks

Mitigation

Don't initialize variables to default value

increments can be unchecked in loops

Summary

Unchecked operations as the ++i on for loops are cheaper than checked one.

Details

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

The code would go from:

for (uint256 i; i < numIterations; i++) {
// ...
}

to

for (uint256 i; i < numIterations;) {
  // ...
  unchecked { ++i; }
}
The risk of overflow is inexistent for a uint256 here.

Github Permalinks

Mitigation

Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

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

Summary

In loops not assigning the length to a variable so memory accessed a lot (caching local variables)

Details

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)

Github Permalinks

Mitigation

Assign the length of the array.length to a local variable in loops for gas savings

Variables should be cached when used several times

Summary

Variables read more than once improves gas usage when cached into local variable

Details

In loops or state variables, this is even more gas saving

Github Permalinks

calls[i] https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L65-L72

params.preciousTokens[i], params.preciousTokenIds[i] https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L80-L83

preciousTokenIds[i] https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/LibProposal.sol#L33

Mitigation

Cache variables used more than one into a local variable.

Shift right instead of dividing by 2

Summary

Shifting is cheaper than dividing by 2

Details

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity’s division operation also includes a division-by-0 prevention which is bypassed using shifting.

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L434 uint256 mid = (low + high) / 2;

Mitigation

Consider replacing / 2 with >> 1 here

>= cheaper than >

Summary

Strict inequalities ( > ) are more expensive than non-strict ones ( >= ). This is due to some supplementary checks (ISZERO, 3 gas)

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L144 if (initialBalance > 0) {

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L471 if (votingPower > 0) {

Mitigation

Consider using >= 1 instead of > 0 to avoid some opcodes

<X> += <Y> costs more gas than <X> = <X> + <Y> for state variables

Summary

x+=y costs more gas than x=x+y for state variables

Github Permalinks

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L243 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L411

Mitigation

Don't use += for state variables as it cost more gas.

PartyDAO contest - Deivitto's results

A protocol for buying, using, and selling NFTs as a group.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-15] QA Report - $91.97

Findings Information

Awards

Labels

External Links

Low

Prevent div by 0

Impact

Proof of Concept

Recommended Mitigation Steps

Variable shadows another variable

Summary

Details

Github Permalinks

Mitigation

Unused receive() function will lock ether in contract

Summary

Github Permalinks

Mitigation

Missing checks for address(0x0) when assigning values to address state or immutable variables

Summary

Github Permalinks

Mitigation

block.timestamp used as time proxy in a strict equality

Summary

Details

References

Github Permalinks

Mitigation

Use of asserts()

Impact

Details

References

Github Permalinks

Recommended Mitigation Steps

Informational

Use of magic numbers is confusing and risky

Summary

Details

Github Permalinks

Mitigation

Missing indexed event parameters

Summary

Details

Github Permalinks

Mitigation

Naming convention of state variable non constant

Summary

Github Permalinks

Mitigation

Missing inheritance

Github permalinks

Mitigation

Different versions of pragma

Summary

Mitigation

Missing onlyConstructor modifier on initialize

Github Permalink

Mitigation

Missing event on interface

Summary

Github Permalinks

Mitigation

Maximum line length exceeded

Summary

Details

Github Permalinks

Mitigation

Missing error messages in require statements

Summary

Github Permalinks

Mitigation

#0 - HardlyDifficult2022-10-04T09:50:54Z

[G-06] Gas Report - $69.83

Unused `receive()` function will lock ether in contract

Missing checks for address(0x0) when assigning values to `address` state or `immutable` variables

#0 - HardlyDifficult
2022-10-04T09:50:54Z