Juicebox V2 contest - Meera's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L44

Vulnerability details

Impact

latestRoundData() is used to fetch the asset price from a Chainlink aggregator, but it's missing additional validations to ensure that the round is complete. If there is a problem with Chainlink starting a new round and finding consensus on the new value for the oracle (e.g. Chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the Chainlink system) consumers of this contract may continue using outdated stale data / stale prices.

Proof of Concept

JBChainlinkV3PriceFeed.sol:44:    (, int256 _price, , , ) = feed.latestRoundData();

Recommended Mitigation Steps

Consider adding missing checks for stale data.

As an example:

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
-    (, int256 _price, , , ) = feed.latestRoundData();
+    (uint80 roundID, int256 _price, , uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData(); //@audit The updatedAt data feed property is the timestamp of an answered round and answeredInRound is the round it was updated in
+    require(updatedAt > 0, "Price data is stale"); //@audit A timestamp with zero value means the round is not complete and should not be used.
+    require(_price > 0, "Price data not valid");
+    require(answeredInRound >= roundID, "Price data is stale"); //@audit If answeredInRound is less than roundID, the _price is being carried over. If answeredInRound is equal to roundID, then the _price is fresh.
    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();

    // Return the price, adjusted to the target decimals.
    return uint256(_price).adjustDecimals(_feedDecimals, _decimals);
  }

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L98-L100 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L518 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1042 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1107 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1226

Vulnerability details

Impact

DOS

Proof of Concept

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

As USDT is an extremely popular ERC20 token, it's quite probable that it'll be used in the JBERC20PaymentTerminal, which implements the JBPayoutRedemptionPaymentTerminal abstract contract and overrides the _beforeTransferTo() function as such:

File: JBERC20PaymentTerminal.sol
19: contract JBERC20PaymentTerminal is JBPayoutRedemptionPaymentTerminal {
...
098:   function _beforeTransferTo(address _to, uint256 _amount) internal override {
099:     IERC20(token).approve(_to, _amount);
100:   }

In the abstract contract JBPayoutRedemptionPaymentTerminal, calls to _beforeTransferTo() are always protected against the 0 value, meaning it won't ever be possible to approve another amount after the initial one. As _beforeTransferTo() is a hook in several functions (migrate(), _distributeToPayoutSplitsOf(), _processFee()), these functions would just revert with USDT or similar tokens.

Mitigation

Consider approving 0 first to avoid this type of DOS:

File: JBERC20PaymentTerminal.sol
098:   function _beforeTransferTo(address _to, uint256 _amount) internal override {
+ 099:     IERC20(token).approve(_to, 0);
099:     IERC20(token).approve(_to, _amount);
100:   }

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L332-L349 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L81-L89 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L540-L555

Vulnerability details

As arbitrary ERC20 tokens can be used, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Affected code:

• Incompatible with FoT tokens

File: JBERC20PaymentTerminal.sol
81:   function _transferFrom(
82:     address _from,
83:     address payable _to,
84:     uint256 _amount
85:   ) internal override {
86:     _from == address(this)
87:       ? IERC20(token).transfer(_to, _amount) //@audit low: result not checked
88:       : IERC20(token).transferFrom(_from, _to, _amount); 
89:   }

• pay()

File: JBPayoutRedemptionPaymentTerminal.sol
332:   function pay(
...
348:       // Transfer tokens to this terminal from the msg sender.
349:       _transferFrom(msg.sender, payable(address(this)), _amount);//@audit FoT incompatible
...
355:       _pay(
356:         _amount, //@audit wrong amount
357:         msg.sender,
358:         _projectId,
359:         _beneficiary,
360:         _minReturnedTokens,
361:         _preferClaimedTokens,
362:         _memo,
363:         _metadata
364:       );

• addToBalanceOf()

File: JBPayoutRedemptionPaymentTerminal.sol
540:   function addToBalanceOf(
...
554:       // Transfer tokens to this terminal from the msg sender.
555:       _transferFrom(msg.sender, payable(address(this)), _amount);//@audit FoT incompatible
...
561:     _addToBalanceOf(_projectId, _amount, !isFeelessAddress[msg.sender], _memo, _metadata); //@audit wrong amount

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter. Or explicitly document that such tokens shouldn't be used and won't be supported

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Check Effects Interactions pattern not respected
  • 1.2. Missing address(0) checks
  • 1.3. Unsafe use of transfer()/transferFrom() with IERC20
  • 1.4. Prevent accidentally burning tokens
  • 1.5. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. Typos
  • 2.2. 1000000000 should be changed to 1e9 for readability reasons
  • 2.3. Use bytes.concat()

1. Low Risk Issues

1.1. Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

While here, there's no real impact, it's still a bad practice (_safeMint has a callback, and a state change is occurring just after the call, albeit under certain conditions and without any thinkable impact here):

• JBProjects.sol#createFor()

File: JBProjects.sol
131:   function createFor(address _owner, JBProjectMetadata calldata _metadata)
132:     external
133:     override
134:     returns (uint256 projectId)
135:   {
136:     // Increment the count, which will be used as the ID.
137:     projectId = ++count;
138: 
+ 139:     // Set the metadata if one was provided.
+ 140:     if (bytes(_metadata.content).length > 0)
+ 141:       metadataContentOf[projectId][_metadata.domain] = _metadata.content;
139:     // Mint the project.
140:     _safeMint(_owner, projectId); //@audit low: beware of _safeMint's callback
141: 
- 142:     // Set the metadata if one was provided.
- 143:     if (bytes(_metadata.content).length > 0)
- 144:       metadataContentOf[projectId][_metadata.domain] = _metadata.content; //@audit low: CEI pattern not respected, but no real impact here, just bad practice. 
145: 
146:     emit Create(projectId, _owner, _metadata, msg.sender);
147:   }

1.2. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• JBSingleTokenPaymentTerminal.sol#token

028:   address public immutable override token;
...
124:   constructor(
125:     address _token,
126:     uint256 _decimals,
127:     uint256 _currency
128:   ) {
+ 129:     require(_token != address(0));
129:     token = _token;
130:     decimals = _decimals;
131:     currency = _currency;
132:   }

1.3. Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead or check the return value.

Bad:

IERC20(token).transferFrom(msg.sender, address(this), amount);

Good (using OpenZeppelin's SafeERC20):

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";

// ...

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

Good (using require):

bool success = IERC20(token).transferFrom(msg.sender, address(this), amount);
require(success, "ERC20 transfer failed");

Consider using safeTransfer/safeTransferFrom or wrap in a require statement here:

• JBERC20PaymentTerminal.sol#_transferFrom()

File: JBERC20PaymentTerminal.sol
81:   function _transferFrom(
82:     address _from,
83:     address payable _to,
84:     uint256 _amount
85:   ) internal override {
86:     _from == address(this)
87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount); 
89:   }

1.4. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

  function _transferFrom(
    address _from,
    address payable _to,
    uint256 _amount
  ) internal override {
    _from == address(this)
      ? IERC20(token).transfer(_to, _amount)
      : IERC20(token).transferFrom(_from, _to, _amount); 
  }

1.5. 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 the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

File: JBToken.sol
207:   function transferOwnership(uint256 _projectId, address _newOwner)
208:     public
209:     virtual
210:     override
211:     onlyOwner
212:   {
213:     _projectId; // Prevents unused var compiler and natspec complaints.
214: 
215:     return super.transferOwnership(_newOwner);
216:   }

2. Non-Critical Issues

2.1. Typos

• convinience

abstract/JBPayoutRedemptionPaymentTerminal.sol:837:      // If the fee is zero or if the fee is being used by an address that doesn't incur fees, set the discount to 100% for convinience.
abstract/JBPayoutRedemptionPaymentTerminal.sol:948:      // If the fee is zero or if the fee is being used by an address that doesn't incur fees, set the discount to 100% for convinience.

• prefered

abstract/JBPayoutRedemptionPaymentTerminal.sol:1077:            // Add to balance if prefered.
abstract/JBPayoutRedemptionPaymentTerminal.sol:1116:            // Add to balance if prefered.

• guage

abstract/JBPayoutRedemptionPaymentTerminal.sol:1470:      // If the guage reverts, set the discount to 0.

• metadta

libraries/JBFundingCycleMetadataResolver.sol:120:    // global metadta in bits 8-23 (16 bits).

• mumber

JBSingleTokenPaymentTerminalStore.sol:384:    // The weight is always a fixed point mumber with 18 decimals. To ensure this, the ratio should use the same number of decimals as the `_amount`.

• areference

JBSingleTokenPaymentTerminalStore.sol:452:        // Get areference to the terminal's currency.

• extention

JBSplitsStore.sol:218:          // Allow lock extention.

2.2. 1000000000 should be changed to 1e9 for readability reasons

libraries/JBConstants.sol:9:  uint256 public constant MAX_RESERVED_RATE = 10000;
libraries/JBConstants.sol:10:  uint256 public constant MAX_REDEMPTION_RATE = 10000;
libraries/JBConstants.sol:11:  uint256 public constant MAX_DISCOUNT_RATE = 1000000000;
libraries/JBConstants.sol:12:  uint256 public constant SPLITS_TOTAL_PERCENT = 1000000000;
libraries/JBConstants.sol:13:  uint256 public constant MAX_FEE = 1000000000;
libraries/JBConstants.sol:14:  uint256 public constant MAX_FEE_DISCOUNT = 1000000000;

2.3. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

abstract/JBPayoutRedemptionPaymentTerminal.sol:2:pragma solidity 0.8.6;
abstract/JBPayoutRedemptionPaymentTerminal.sol:1075:            _projectMetadata = bytes(abi.encodePacked(_projectId));
abstract/JBPayoutRedemptionPaymentTerminal.sol:1114:            _projectMetadata = bytes(abi.encodePacked(_projectId));

Overview

Table of Contents:

• 1. Multiple accesses of a mapping/array should use a local variable cache
• 2. Cheap Contract Deployment Through Clones
• 3. Amounts should be checked for 0 before calling a transfer
• 4. <array>.length should not be looked up in every loop of a for-loop
• 5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 6. Increments can be unchecked in for-loops
• 7. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied
• 8. Upgrade pragma
• 9. Functions guaranteed to revert when called by normal users can be marked payable

1. Multiple accesses of a mapping/array should use a local variable cache

Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.

Affected code:

File: JBOperatorStore.sol
134:   function setOperators(JBOperatorData[] calldata _operatorData) external override {
135:     for (uint256 _i = 0; _i < _operatorData.length; _i++) {
136:       // Pack the indexes into a uint256.
+ 136:       JBOperatorData calldata _operatorDataI = _operatorData[_i];
- 137:       uint256 _packed = _packedPermissions(_operatorData[_i].permissionIndexes);
+ 137:       uint256 _packed = _packedPermissions(_operatorDataI.permissionIndexes);
138: 
139:       // Store the new value.
- 140:       permissionsOf[_operatorData[_i].operator][msg.sender][_operatorData[_i].domain] = _packed;
+ 140:       permissionsOf[_operatorDataI.operator][msg.sender][_operatorDataI.domain] = _packed;
141: 
142:       emit SetOperator(
- 143:         _operatorData[_i].operator,
+ 143:         _operatorDataI.operator,
144:         msg.sender,
- 145:         _operatorData[_i].domain,
+ 145:         _operatorDataI.domain,
- 146:         _operatorData[_i].permissionIndexes,
+ 146:         _operatorDataI.permissionIndexes,
147:         _packed
148:       );
149:     }
150:   }

2. Cheap Contract Deployment Through Clones

JBTokenStore.sol:203:    token = new JBToken(_name, _symbol);

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

Keep in mind however, that the gas saving only concerns the deployment cost. Indeed, by using this pattern, the cost to use the deployed contract increases a bit.

Depending on the sponsor's choice (saving a bit of gas for the end-users or a lot of gas for the deployer), this pattern might be considered.

3. Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

Consider adding a non-zero-value check here:

File: JBERC20PaymentTerminal.sol
81:   function _transferFrom(
82:     address _from,
83:     address payable _to,
84:     uint256 _amount
85:   ) internal override {
86:     _from == address(this)
87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount); 
89:   }

4. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {

5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

6. Increments can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The risk of overflow is non-existant for uint256 here.

7. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied

If the optimizer is enabled, this finding isn't true anymore

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas (around 3 gas per instance).

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

abstract/JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBProjects.sol:40:  uint256 public override count = 0;
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:209:      bool _includesLocked = false;
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:227:    uint256 _percentTotal = 0;
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Consider removing explicit initializations for default values.

8. Upgrade pragma

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Contract existence checks (>= 0.8.10): external calls skip contract existence checks if the external call has a return value

Consider upgrading here :

abstract/JBControllerUtility.sol:2:pragma solidity 0.8.6;
abstract/JBOperatable.sol:2:pragma solidity 0.8.6;
abstract/JBPayoutRedemptionPaymentTerminal.sol:2:pragma solidity 0.8.6;
abstract/JBSingleTokenPaymentTerminal.sol:2:pragma solidity 0.8.6;
JBChainlinkV3PriceFeed.sol:2:pragma solidity 0.8.6;
JBController.sol:2:pragma solidity 0.8.6;
JBDirectory.sol:2:pragma solidity 0.8.6;
JBERC20PaymentTerminal.sol:2:pragma solidity 0.8.6;
JBETHPaymentTerminal.sol:2:pragma solidity 0.8.6;
JBFundingCycleStore.sol:2:pragma solidity 0.8.6;
JBOperatorStore.sol:2:pragma solidity 0.8.6;
JBPrices.sol:2:pragma solidity 0.8.6;
JBProjects.sol:2:pragma solidity 0.8.6;
JBReconfigurationBufferBallot.sol:2:pragma solidity 0.8.6;
JBSingleTokenPaymentTerminalStore.sol:2:pragma solidity 0.8.6;
JBSplitsStore.sol:2:pragma solidity 0.8.6;
JBToken.sol:2:pragma solidity 0.8.6;
JBTokenStore.sol:2:pragma solidity 0.8.6;

9. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

abstract/JBControllerUtility.sol:8:  Provides tools for contracts with functionality that can only be accessed by a project's controller.
abstract/JBPayoutRedemptionPaymentTerminal.sol:622:  function setFee(uint256 _fee) external virtual override onlyOwner {
abstract/JBPayoutRedemptionPaymentTerminal.sol:644:  function setFeeGauge(IJBFeeGauge _feeGauge) external virtual override onlyOwner {
abstract/JBPayoutRedemptionPaymentTerminal.sol:661:  function setFeelessAddress(address _address, bool _flag) external virtual override onlyOwner {
JBController.sol:28:  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.
JBProjects.sol:179:  function setTokenUriResolver(IJBTokenUriResolver _newResolver) external override onlyOwner {