Juicebox contest - __141345__'s results

Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances number of this issue:

// contracts/interfaces/IJB721TieredGovernance.sol
16-22:
  event TierDelegateVotesChanged(
    address indexed delegate,
    uint256 indexed tierId,
    uint256 previousBalance,
    uint256 newBalance,
    address callre
  );

// contracts/interfaces/IJBTiered721Delegate.sol
29:   event AddTier(uint256 indexed tierId, JB721TierParams data, address caller);

// contracts/interfaces/IJBTiered721DelegateDeployer.sol
9-13:
  event DelegateDeployed(
    uint256 indexed projectId,
    IJBTiered721Delegate newDelegate,
    JB721GovernanceType governanceType
  );

require()/revert() statements should have descriptive reason strings

// contracts/JBTiered721Delegate.sol
216:    require(address(this) != codeOrigin);
218:    require(address(store) == address(0));

Use encode instead of encodePacked for hashing

Use of abi.encodePacked is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

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

Instances number of this issue:

// contracts/libraries/JBIpfsDecoder.sol
28:     bytes memory completeHexString = abi.encodePacked(bytes2(0x1220), _hexString);
34:     return string(abi.encodePacked(_baseUri, ipfsHash));

Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

Instances number of this issue: 31

// contracts/JB721TieredGovernance.sol
44:   mapping(address => mapping(uint256 => address)) internal _tierDelegation;
53:   mapping(address => mapping(uint256 => Checkpoints.History)) internal _delegateTierCheckpoints;
61:   mapping(uint256 => Checkpoints.History) internal _totalTierCheckpoints;

// contracts/JBTiered721DelegateStore.sol
57:   mapping(address => mapping(uint256 => uint256)) internal _tierIdAfter;
66:   mapping(address => mapping(uint256 => address)) internal _reservedTokenBeneficiaryOf;
75:   mapping(address => mapping(uint256 => JBStored721Tier)) internal _storedTierOf;
83:   mapping(address => JBTiered721Flags) internal _flagsOf;
93:   mapping(address => mapping(uint256 => uint256)) internal _isTierRemoved;
104:  mapping(address => uint256) internal _trackedLastSortTierIdOf;

119:  mapping(address => uint256) public override maxTierIdOf;
129:  mapping(address => mapping(address => mapping(uint256 => uint256))) public override tierBalanceOf;
138:  mapping(address => mapping(uint256 => uint256)) public override numberOfReservesMintedFor;
147:  mapping(address => mapping(uint256 => uint256)) public override numberOfBurnedFor;
155:  mapping(address => address) public override defaultReservedTokenBeneficiaryOf;
164:  mapping(address => mapping(uint256 => address)) public override firstOwnerOf;
172:  mapping(address => string) public override baseUriOf;
180:  mapping(address => IJBTokenUriResolver) public override tokenUriResolverOf;
188:  mapping(address => string) public override contractUriOf;
197:  mapping(address => mapping(uint256 => bytes32)) public override encodedIPFSUriOf;

internal functions only called once can be inlined

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Instances number of this issue: 9

// contracts/JB721TieredGovernance.sol

  function _getTierVotingUnits(address _account, uint256 _tierId)
    internal
    view
    virtual
    returns (uint256)

  function _transferTierVotingUnits(
    address _from,
    address _to,
    uint256 _tierId,
    uint256 _amount
  ) internal virtual {

// contracts/JBTiered721Delegate.sol
  function _mintBestAvailableTier(
    uint256 _amount,
    address _beneficiary,
    bool _expectMint
  ) internal returns (uint256 leftoverAmount) {

  function _mintAll(
    uint256 _amount,
    uint16[] memory _mintTierIds,
    address _beneficiary
  ) internal returns (uint256 leftoverAmount) {

  function _afterTokenTransferAccounting(
    address _from,
    address _to,
    uint256 _tokenId,
    JB721Tier memory _tier
  ) internal virtual {

// contracts/JBTiered721DelegateDeployer.sol
  function _clone(address _targetAddress) internal returns (address _out) {

// contracts/JBTiered721DelegateProjectDeployer.sol
  function _launchProjectFor(address _owner, JBLaunchProjectData memory _launchProjectData)
    internal

  function _launchFundingCyclesFor(
    uint256 _projectId,
    JBLaunchFundingCyclesData memory _launchFundingCyclesData
  ) internal returns (uint256) {

  function _reconfigureFundingCyclesOf(
    uint256 _projectId,
    JBReconfigureFundingCyclesData memory _reconfigureFundingCyclesData
  ) internal returns (uint256) {

Duplicate check for _leftoverAmount

Line 582, _leftoverAmount can not be 0 because of line 575.

// contracts/JBTiered721Delegate.sol
575:    if (_leftoverAmount != 0) {
          // ...
582:      if (_leftoverAmount != 0) {

Named return variables

Adding a return statement when the function defines a named return variable, is redundant

Consider changing the variable to be an unnamed one.

Instances number of this issue:

// contracts/JBTiered721Delegate.sol
  function _mintBestAvailableTier(
    uint256 _amount,
    address _beneficiary,
    bool _expectMint
  ) internal returns (uint256 leftoverAmount) {
631:          return leftoverAmount;
  }

Unnecessary assignment

tokenId = _tierId is not needed.

// contracts/JBTiered721DelegateStore.sol
  function _generateTokenId(uint256 _tierId, uint256 _tokenNumber)
    internal
    pure
    returns (uint256 tokenId)
  {
    // The tier ID in the first 16 bits.
    tokenId = _tierId;

    // The token number in the rest.
    tokenId |= _tokenNumber << 16;
  }