QA Report - Githubissues

QA Report low

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

Checks are missing for all three variables below:

    globalGovernance = _globalGovernance;
    tieredGovernance = _tieredGovernance;
    noGovernance = _noGovernance;

QA Report: non-critical

Typos

JBTiered721DelegateDeployer.sol: L132

      // Copy the bytecode (our initialise part is 13 bytes long)

Change initialise to initialize. The spelling of a given word should be consistent. Most of the contest uses the American English spelling of initialize. For example, JBTiered721DelegateDeployer.sol: L84. The following six additional instances of initialise also should be corrected:

JBTiered721DelegateStore.sol: L233

JBTiered721DelegateStore.sol: L716

JBTiered721DelegateStore.sol: L947

JBTiered721DelegateStore.sol: L1032

JBTiered721DelegateStore.sol: L1183

JBBitmap.sol: L13

JBTiered721DelegateProjectDeployer.sol: L19

  JBOperatable: Several functions in this contract can only be accessed by a project owner, or an address that has been preconfifigured to be an operator of the project.

Change preconfifigured to preconfigured

JBTiered721DelegateProjectDeployer.sol: L34

    The contract responsibile for deploying the delegate.

Change responsibile to responsible

JBTiered721Delegate.sol: L17

  Delegate that offers project contributors NFTs with tiered price floors upon payment and the ability to redeem NFTs for treasury assets based based on price floor.

Remove repeated word based

The same typo (accross) occurs in both lines below:

JBTiered721Delegate.sol: L121

JBTiered721DelegateStore.sol: L497

    @return balance The number of tokens owners by the owner accross all tiers.

Change accross to across in both cases

The same typo (adherance) occurs in both lines below:

JBTiered721Delegate.sol: L173

JB721Delegate.sol: L176

    @param _interfaceId The ID of the interface to check for adherance to.

Change adherance to adherence in both cases

JBTiered721Delegate.sol: L268

    // Keep a reference to the number of tiers there are to mint reserved for.

Change reserved to reserves

The same typo (beneificiary) occurs in both lines below:

JBTiered721Delegate.sol: L363

JBTiered721Delegate.sol: L368

    Sets the beneificiary of the reserved tokens for tiers where a specific beneficiary isn't set.

Change beneificiary to beneficiary in both cases

JBTiered721Delegate.sol: L545

    // Keep a reference to the flag indicating if the transaction should revert if all provded funds aren't spent. Defaults to false, meaning only a minimum payment is enforced.

Change provded to provided

JBTiered721Delegate.sol: L557

      // Keep a reference to the the specific tier IDs to mint.

Remove repeated word the

JBTiered721Delegate.sol: L717

    User the hook to register the first owner if it's not yet regitered.

Change User to Use and regitered to registered

JBTiered721DelegateStore.sol: L176

    Custom token URI resolver, superceeds base URI.

Change superceeds to supersedes

JBTiered721DelegateStore.sol: L230

    // Keep a referecen to the tier being iterated on.

Change referecen to reference

JBTiered721DelegateStore.sol: L597

    @return The reserved token benficiary.

Change benficiary to beneficiary

JBTiered721DelegateStore.sol: L719

        // Keep a reference to the idex to iterate on next.

Change idex to index

JBTiered721DelegateStore.sol: L832

    // Keep a reference to the number of burned in the tier.

Change of burned to burned

JBTiered721DelegateStore.sol: L852

    @param _beneficiary The reservd token beneficiary.

Change reservd to reserved

JB721Delegate.sol: L88

    // Forward the recieved weight and memo, and use this contract as a pay delegate.

Change recieved to received

JB721Delegate.sol: L144

    // These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.

Change operation to operations

JBIpfsDecoder.sol: L42

    Written by Martin Ludfall - Licence: MIT

Change Licence to License

NatSpec statements are missing

NatSpec statements are entirely missing for the two constructors referenced below:

JBTiered721DelegateDeployer.sol: L46-54

  constructor(
    JB721GlobalGovernance _globalGovernance,
    JB721TieredGovernance _tieredGovernance,
    JBTiered721Delegate _noGovernance
  ) {
    globalGovernance = _globalGovernance;
    tieredGovernance = _tieredGovernance;
    noGovernance = _noGovernance;
  }

JBTiered721Delegate.sol: L185-187

  constructor() {
    codeOrigin = address(this);
  }

NatSpec statements are also entirely missing for the four functions below:

JBTiered721DelegateDeployer.sol: L115-118

  function _clone(address _targetAddress) internal returns (address _out) {
    assembly {
      // Get deployed/runtime code size
      let _codeSize := extcodesize(_targetAddress)

JBTiered721FundingCycleMetadataResolver.sol: L6-9

library JBTiered721FundingCycleMetadataResolver {
  function transfersPaused(uint256 _data) internal pure returns (bool) {
    return (_data & 1) == 1;
  }

JBTiered721FundingCycleMetadataResolver.sol: L11-13

  function mintingReservesPaused(uint256 _data) internal pure returns (bool) {
    return ((_data >> 1) & 1) == 1;
  }

JBTiered721FundingCycleMetadataResolver.sol: L15-17

  function changingPricingResolverPaused(uint256 _data) internal pure returns (bool) {
    return ((_data >> 2) & 1) == 1;
  }

NatSpec is incomplete

The following functions are missing @return:

JB721TieredGovernance.sol: L68-81

JB721TieredGovernance.sol: L84-92

JB721TieredGovernance.sol: L95-108

JB721TieredGovernance.sol: L111-118

JB721TieredGovernance.sol: L121-135

JBTiered721Delegate.sol: L167-179

JB721Delegate.sol: L170-191

Long single-line comments

In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comments interfere with readability. Below are five instances of extra-long comments whose readability could be improved by wrapping, as shown:

JBTiered721DelegateDeployer.sol: L15

  IJBTiered721DelegateDeployer: General interface for the generic controller methods in this contract that interacts with funding cycles and tokens according to the protocol's rules.

Suggestion:

  IJBTiered721DelegateDeployer: General interface for the generic controller methods in this contract 
      that interacts with funding cycles and tokens according to the protocol's rules.

Similarly for the following equivalent comment: JBTiered721DelegateProjectDeployer.sol: L15

JB721TieredGovernance.sol: L235

    Transfers, mints, or burns tier voting units. To register a mint, `from` should be zero. To register a burn, `to` should be zero. Total supply of voting units will be adjusted with mints and burns.

Suggestion:

    Transfers, mints, or burns tier voting units. To register a mint, `from` should be zero. 
        To register a burn, `to` should be zero. Total supply of voting units will be adjusted with mints and burns.

JBTiered721Delegate.sol: L198

    @param _pricing The tier pricing according to which token distribution will be made. Must be passed in order of contribution floor, with implied increasing value.

Suggestion:

    @param _pricing The tier pricing according to which token distribution will be made. 
        Must be passed in order of contribution floor, with implied increasing value.

JBTiered721Delegate.sol: L545

    // Keep a reference to the flag indicating if the transaction should revert if all provded funds aren't spent. Defaults to false, meaning only a minimum payment is enforced.

Suggestion:

    // Keep a reference to the flag indicating if the transaction should revert if all provided funds aren't spent. 
    //    Defaults to false, meaning only a minimum payment is enforced.

JB721Delegate.sol: L144

    // These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.

Suggestion:

    // These conditions are all part of the same curve. Edge conditions
    //    are separated because fewer operation are necessary.

Open items

Comments that contain language referring to possible open items should be addressed and modified or removed

JBTiered721DelegateProjectDeployer.sol: L75

    // Get the project ID, optimistically knowing it will be one greater than the current count.

While it might just be an example of awkward language, the phrase "optimistically knowing" appears ambiguous and should be reviewed and corrected

Use scientific notation for large multiples of 10 rather than decimal literals

For readability, used scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000) for large multiples of ten

JBTiered721DelegateDeployer.sol: L123-124

      // Shift the length to the length placeholder, in the constructor
      let _mask := mul(_codeSize, 0x100000000000000000000000000000000000000000000000000000000)

Missing `require` revert string

An error message should be included to enable users to understand the reason for failure.

Recommendation: Add a brief (<= 32 char) explanatory message to both of the requires referenced below. Or use custom error codes (which would be cheaper than revert strings).

JBTiered721Delegate.sol: L216

    require(address(this) != codeOrigin);

JBTiered721Delegate.sol: L218

    require(address(store) == address(0));

Use consistent initialization of counters in `for` loops

Most for loop counters in Juicebox are not initiated to zero whereas a few are. It is not necessary to initialize for loop counters to zero since this is their default value.

For consistency, it makes sense to omit counter initializations in the for loops below.

JBIpfsDecoder.sol: L49-62

    for (uint256 i = 0; i < _source.length; ++i) {
      uint256 carry = uint8(_source[i]);
      for (uint256 j = 0; j < digitlength; ++j) {
        carry += uint256(digits[j]) * 256;
        digits[j] = uint8(carry % 58);
        carry = carry / 58;
      }

      while (carry > 0) {
        digits[digitlength] = uint8(carry % 58);
        digitlength++;
        carry = carry / 58;
      }
    }

JBIpfsDecoder.sol: L68-70

    for (uint256 i = 0; i < _length; i++) {
      output[i] = _array[i];
    }

JBIpfsDecoder.sol: L76-78

    for (uint256 i = 0; i < _input.length; i++) {
      output[i] = _input[_input.length - 1 - i];
    }

JBIpfsDecoder.sol: L84-86

    for (uint256 i = 0; i < _indices.length; i++) {
      output[i] = _ALPHABET[_indices[i]];
    }

code-423n4 / 2022-10-juicebox-findings

QA Report #188

QA Report low

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

QA Report: non-critical

Typos

NatSpec statements are missing

NatSpec is incomplete

Long single-line comments

Open items

Use scientific notation for large multiples of 10 rather than decimal literals

Missing `require` revert string

Use consistent initialization of counters in `for` loops

code-423n4 / 2022-10-juicebox-findings

QA Report #188

QA Report low

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

QA Report: non-critical

Typos

NatSpec statements are missing

NatSpec is incomplete

Long single-line comments

Open items

Use scientific notation for large multiples of 10 rather than decimal literals

Missing require revert string

Use consistent initialization of counters in for loops

Missing `require` revert string

Use consistent initialization of counters in `for` loops