GalloDaSballo
commented
1 year ago [L01] Missing checks for address(0x0) when assigning values to address state variables
L
[L02]initialize functions can be front-run
Disputed for majority, invalid
[L03] Unused receive() function will lock Ether in contract
Disputed, see execute
[L04] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Disputed, for the examples above Please provide a clashing hash next time
[L05]_safeMint() should be used rather than _mint() wherever possible
Disputed
[L06] Unspecific Compiler Version Pragma
NC
[L07] Use Two-Step Transfer Pattern for Access Controls
NC
[N01] Adding a return statement when the function defines a named return variable, is redundant
Invalid, those are type modifiers, your grep checks for 2 words in the return that's not sufficient
[N02] require()/revert() statements should have descriptive reason strings
Disagree, the code is returning the reason
Disputed rest
[N03] constants should be defined rather than using magic numbers
R
[N04] Use a more recent version of solidity
NC
[N05] Variable names that consist of all capital letters should be reserved for const/immutable variables
R
[N06] Event is missing indexed fields
NC
[N07] Use of sensitive/NC-inclusive terms
Disputed
[N08] States/flags should use Enums rather than separate constants
Disputed for this specific case
[L01] Missing checks for
address(0x0)when assigning values toaddressstate variablesFindings:
[L02]
initializefunctions can be front-runImpact
See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
Findings:
[L03] Unused
receive()function will lock Ether in contractImpact
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert
Findings:
[L04]
abi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256()Impact
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
Findings:
[L05]
_safeMint()should be used rather than_mint()wherever possibleImpact
Issue Information: [L022](https://github.com/Bnke0x0/c4-common-issues/blob/main/2-Low-Risk.md#l022---_safeMint()-should-be-used-rather-than-_mint()-wherever-possible)
Findings:
[L06] Unspecific Compiler Version Pragma
Impact
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
Findings:
[L07] Use Two-Step Transfer Pattern for Access Controls
Impact
Contracts implementing access control's, e.g.
owner, should consider implementing a Two-Step Transfer pattern.Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role.
Findings:
Non-Critical Issues
[N01] Adding a
returnstatement when the function defines a named return variable, is redundantFindings:
[N02]
require()/revert()statements should have descriptive reason stringsImpact
Issue Information: [NC002](https://github.com/Bnke0x0/c4-common-issues/blob/main/2-Low-Risk.md#n002---require()/revert()-statements-should-have-descriptive-reason-strings)
Findings:
[N03] constants should be defined rather than using magic numbers
Impact
Issue Information: NC003
Findings:
[N04] Use a more recent version of solidity
Findings:
[N05] Variable names that consist of all capital letters should be reserved for
const/immutablevariablesImpact
If the variable needs to be different based on which class it comes from, a view/pure function should be used instead
Findings:
[N06] Event is missing
indexedfieldsImpact
Each event should use three indexed fields if there are three or more fields
Findings:
[N07] Use of sensitive/NC-inclusive terms
Findings:
[N08] States/flags should use Enums rather than separate constants
Findings:
[N09] Unused file
Findings:
[N10]
publicfunctions not called by the contract should be declaredexternalinsteadImpact
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
Findings:
[N11] Numeric values having to do with time should use time units for readability
Impact
There are units for seconds, minutes, hours, days, and weeks
Findings:
[N12] Constant redefined elsewhere
Impact
Consider defining in only one contract so that values cannot become out of sync when only one location is updated
Findings:
[N13] NC-library/interface files should use fixed compiler versions, not floating ones
Findings: