PartyDAO contest - CodingNameKiki'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: 28/110

Findings: 2

Award: $151.26

🌟 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

115.9125 USDC - $115.91

Labels

bug

low quality report

QA (Quality Assurance)

sponsor disputed

edited-by-warden

External Links

Link to GitHub issue

Low risk Issues - 69 instances found over 13 Low risk Issues

L-01. Assert Violation

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:

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

Recommended: Consider whether the condition checked in the assert() is actually an invariant. If not, replace the assert() statement with a require() statement.If the exception is indeed caused by unexpected behaviour of the code, fix the underlying bug(s) that allow the assertion to be violated.

L-02. Missing checks for address(0x0) when assigning values to address state variables

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters. Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

L-03. Use of floating pragma

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.

Recommended: Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

L-04. Several critical operations do not trigger events, which will make it difficult to review the correct behavior of the contracts once deployed.

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/globals/Globals.sol#L27 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L653 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L783 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L262 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L123 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L116 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L93 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L78

L-05. _safeMint() should be used rather than _mint, whenever possible.

_mint() is discouraged 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/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L271

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

L-06. Return values of approve() not checked

Not all implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L256 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnOpenseaProposal.sol#L367 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ListOnZoraProposal.sol#L143

L-07. Unused empty receive()/fallback() 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)))

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

L-08. Setters should check the input value.

Add a non-zero address check to the function transferMultiSig(), so it can revert if the newMultisag is a zero address.

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

L-09. Tokens can be sent to the wrong address

The tokens in the function _transfer() can be sent to the zero address, there is no zero-address check to ensure the tokens are sent to the right address.

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

L-10. The else statement is wrongly applied

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

The else statement in the function _transfer() can be better applied. Currently the else statement uses assert to make sure the token type is ERC20. The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. So instead of using assert, we can just replace all of that with: else if (tokenType == TokenType.Erc20) { IERC20(token).compatTransfer(recipient, amount); }

L-11. Low level calls don't check for contract existence

Low level calls return success if called on a destructed contract. See OpenZeppelin’s Address.sol which checks address.code.length

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L795 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L131

L-12. Move ETH constant to child contract

All of the functions in the TokenDistributor require 0x0 when referring to Ether, not the constant here. Having it here will lead to mistakes down the line. It’s only used by createNativeDistribution(), so it only needs to be there (it currently is also defined there).

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

L-13. Payable function when using ERC721

There should be a require(0 == msg.value) to ensure no Ether is being sent to the function when the currency used in an order is a ERC721 token.

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

Non-critical Issues - 104 instances found over 12 Non-critical Issues

N-01. Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents functions and change the visibility from external to public. https://docs.soliditylang.org/en/latest/contracts.html#function-overriding

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L167 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L191 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L138 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L88 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L35 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L61 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/CrowdfundFactory.sol#L87

N-02. Constants should be defined rather than using magic numbers

N-03. Use two-phase ownership transfers

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

N-04. Function Naming suggestions

Proper use of _ as a function name prefix A common pattern is to prefix internal and private function names with _. This pattern is correctly applied in the Party contracts, however there are some inconsistencies in the libraries.

N-05. Proper use of get as a function name prefix

Clear function names can increase readability. Follow a standard convertion function names such as using get for getter (view/pure) functions.

N-06. Expressions for constant values such as a call to keccak256() is prefered to use immutable rather than constant.

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

N-07. Event indexing

Events should use indexed fields

N-08. Inconsistent use of named return variables

There is an inconsistent use of named return variables in the ArbitraryCallsProposal contract.

Some functions return named variables, others return explicit values.

Following function return explicit value: https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ArbitraryCallsProposal.sol#L134-L200

Following function return return a named variable: https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/ArbitraryCallsProposal.sol#L203-L218

Recommended: Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

N-09. Override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernanceNFT.sol#L88 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/AuctionCrowdfund.sol#L283 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ProposalExecutionEngine.sol#L99 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/BuyCrowdfundBase.sol#L166

N-10. Function missing comments.

Some functions are missing comments

N-11. Require() statements should have descriptive reason strings

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

N-12. Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.

#0 - 0xble
2022-09-26T01:30:37Z

Unhelpful bot report with some suggestions that don't make sense.

#1 - HardlyDifficult
2022-09-30T21:23:02Z

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

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

35.3477 USDC - $35.35

Labels

bug

G (Gas Optimization)

low quality report

sponsor acknowledged

External Links

Link to GitHub issue

Use a memory location instead of storage

Using storage pointer instead of memory location can save a decent amount of gas.

a) Few functions in the contract PartyGovernance, use a memory location to retrieve VotingPowerSnapshot

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L227 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L366 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L893 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L936 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L938 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L951 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L961

b) a function uses a memory location to retrieve GovernanceValues

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

c) two functions use a memory location to retrieve

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

No need to explicitly initialize variables with default values

Arrays in foor loops can be cached to save gas.

#0 - 0xble
2022-09-26T01:31:16Z

Unhelpful bot report.

PartyDAO contest - CodingNameKiki's results

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

General Information

Findings Distribution

Researcher Performance

External Links

[Q-06] QA Report - $115.91

Findings Information

Awards

Labels

External Links

#0 - 0xble2022-09-26T01:30:37Z

#1 - HardlyDifficult2022-09-30T21:23:02Z

[G-77] Gas Report - $35.35

Findings Information

Awards

Labels

External Links

#0 - 0xble2022-09-26T01:31:16Z

#0 - 0xble
2022-09-26T01:30:37Z

#1 - HardlyDifficult
2022-09-30T21:23:02Z

#0 - 0xble
2022-09-26T01:31:16Z