QA Report

[code423n4]

• Low
  • 1. Outdated packages
  • 2. Use string.concat instead of abi.encodePacked
  • 3. Integer overflow by unsafe casting
  • 4. Lack of checks supportsInterface
  • 5. Lack of initialize protections

Low

1. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "^4.6.0"

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/releases

2. Use string.concat instead of abi.encodePacked

Tthere is a discussion about removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• JBIpfsDecoder.sol:28
• JBIpfsDecoder.sol:34

3. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

  function tierIdOfToken(uint256 _tokenId) public pure override returns (uint256) {
    // The tier ID is in the first 16 bits.
    return uint256(uint16(_tokenId));
  }

Affected source code:

• JBTiered721DelegateStore.sol:587
• JBTiered721DelegateStore.sol:689
• JBTiered721DelegateStore.sol:690
• JBTiered721DelegateStore.sol:691
• JBTiered721DelegateStore.sol:692
• JBTiered721DelegateStore.sol:693
• JBTiered721DelegateStore.sol:694
• JBIpfsDecoder.sol:50

4. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• JB721Delegate.sol:212
• JBTiered721DelegateDeployer.sol:51-53
• JBTiered721DelegateProjectDeployer.sol:52-53

5. Lack of initialize protections

The following contracts are updateable, and follow the update pattern, these contracts contain an initialize method to set the contract once, but anyone can call this method, this will spend project fuel if someone tries to initialize it before the project.

It is recommended to check the sender when initializing the contract.

Affected source code:

• JBTiered721Delegate.sol:202-214

[simon-something]

4 and 5 are pretty generic, feels like it is not for our codebase (EIP165 is implemented in the contract quoted and the delagate is not updatable, initialize is to palliate the lack of constructor, as the contract is cloned)

[Picodes]

1, 2, 3, 4 are valid. For 2, the comment mentions abi.encode which is not the same as string.concat or abi.encodePacked

[c4-judge]

Picodes marked the issue as grade-b