QA Report #71
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Comments
code423n4
added
bug
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Overview
Table of Contents
@openzeppelin/contractsversionpragma experimental ABIEncoderV2is deprecated_safeMint()should be used rather than_mint()wherever possiblerequire()should be used for checking error conditions on inputs and return values whileassert()should be used for invariant checkingnow) as variable namesabi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256()constantinstead of duplicating the same stringstring.concat()orbytes.concat()returnstatement when the function defines a named return variable, is redundant1. Low Risk Issues
1.1. Critical known vulnerabilities exist in currently used
@openzeppelin/contractsversionAs several known critical vulnerabilities exist in the current
@openzeppelin/contractsversion, consider updatingpackage.jsonwith at least@openzeppelin/[email protected]here:While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution or a future update)
1.2.
pragma experimental ABIEncoderV2is deprecatedUse
pragma abicoder v2instead1.3. Unsafe casting may overflow
SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.
Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:
1.4. Missing address(0) checks
Consider adding an
address(0)check for immutable variables:25: BaseRegistrarImplementation immutable base; 26: IPriceOracle public immutable prices; 27: uint256 public immutable minCommitmentAge; 28: uint256 public immutable maxCommitmentAge; 29: ReverseRegistrar public immutable reverseRegistrar; 30: INameWrapper public immutable nameWrapper; ... 49: constructor( 50: BaseRegistrarImplementation _base, 51: IPriceOracle _prices, 52: uint256 _minCommitmentAge, 53: uint256 _maxCommitmentAge, 54: ReverseRegistrar _reverseRegistrar, 55: INameWrapper _nameWrapper 56: ) { 57: require(_maxCommitmentAge > _minCommitmentAge); + 58: require(_base != address(0)); + 58: require(_prices != address(0)); + 58: require(_reverseRegistrar != address(0)); + 58: require(_nameWrapper != address(0)); 58: 59: base = _base; 60: prices = _prices; 61: minCommitmentAge = _minCommitmentAge; 62: maxCommitmentAge = _maxCommitmentAge; 63: reverseRegistrar = _reverseRegistrar; 64: nameWrapper = _nameWrapper; 65: }35: ENS public immutable override ens; 36: IBaseRegistrar public immutable override registrar; ... 49: constructor( 50: ENS _ens, 51: IBaseRegistrar _registrar, 52: IMetadataService _metadataService 53: ) { + 54: require(_ens != address(0)); + 54: require(_registrar != address(0)); 54: ens = _ens; 55: registrar = _registrar;1.5.
_safeMint()should be used rather than_mint()wherever possible_mint()is discouraged in favor of_safeMint()which ensures that the recipient is either an EOA or implementsERC1155TokenReceiver'sonERC1155Received.Be careful however to respect the CEI pattern or add a re-entrancy guard as
_safeMintadds a callback-check and a maliciousonERC1155Receivedcould be exploited if not careful (the CEIP is respected here).Reading material:
1.6. Use a 2-step ownership transfer pattern
Contracts inheriting from OpenZeppelin's libraries have the default
transferOwnership()function (a one-step process). It's possible that theonlyOwnerrole mistakenly transfers ownership to a wrong address, resulting in a loss of theonlyOwnerrole.Consider overriding the default
transferOwnership()function to first nominate an address as thependingOwnerand implementing anacceptOwnership()function which is called by thependingOwnerto confirm the transfer.1.7.
require()should be used for checking error conditions on inputs and return values whileassert()should be used for invariant checkingProperly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:
As the Solidity version is > 0.8.* the remaining gas would still be refunded in case of failure.
2. Non-Critical Issues
2.1. Avoid using deprecated keywords (
now) as variable namesnowis a deprecated keyword that was used beforeblock.timestamp. Here, a variable is named as "now", which introduces some misunderstanding from the IDE:Consider changing the variable's name.
2.2.
abi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256()Similar issue in the past: here
Original issue: Hash collisions when using abi.encodePacked() with multiple variable length arguments
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), butabi.encode(0x123,0x456)=>0x0...1230...456). If there is only one argument toabi.encodePacked()it can often be cast tobytes()orbytes32()instead.Here, no attack vector can be thought of. It's a simple mispractice, hence the NC severity.
2.3. It's better to emit after all processing is done
2.4. Typos
2.5. Use a
constantinstead of duplicating the same string2.6. Open TODOS
Consider resolving the TODOs before deploying.
2.7. Use
string.concat()orbytes.concat()Solidity version 0.8.4 introduces
bytes.concat()(vsabi.encodePacked(<bytes>,<bytes>))Solidity version 0.8.12 introduces
string.concat()(vsabi.encodePacked(<str>,<str>))2.8. Adding a
returnstatement when the function defines a named return variable, is redundantWhile not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.
Affected code:
2.9. Non-library/interface files should use fixed compiler versions, not floating ones
The text was updated successfully, but these errors were encountered: