Juicebox V2 contest - joestakey's results

QA Report

Table of Contents

• summary
• Function reverting for some tokens
• Immutable addresses lack zero-address check
• Potential revert on transfer with IERC20
• Return value of transfer not checked
• Comment Missing function parameter
• Return statements
• Return value of approve not checked
• Scientific notation
• Timelock for critical parameter change
• Transfer of ownership can be two-steps

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the unsafe use of IERC20 methods.

Function reverting for some tokens

PROBLEM

approve() reverts if called on a non-zero allowance without approving 0 first for some tokens (e.g: USDT). This means the logic in JBERC20PaymentTerminal will not work for these tokens

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBERC20PaymentTerminal.sol

99:     IERC20(token).approve(_to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Consider adding an extra step to approve for _amount = 0 before approving the new _amount

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

JBTokenStore.sol

160:   constructor(
161:     IJBOperatorStore _operatorStore,
162:     IJBProjects _projects,
163:     IJBDirectory _directory
164:   ) JBOperatable(_operatorStore) JBControllerUtility(_directory) {
165:     projects = _projects;
166:   }

JBSplitsStore.sol

120:   constructor(
121:     IJBOperatorStore _operatorStore,
122:     IJBProjects _projects,
123:     IJBDirectory _directory
124:   ) JBOperatable(_operatorStore) {
125:     projects = _projects;
126:     directory = _directory;
127:   }

JBDirectory.sol

182:   constructor(
183:     IJBOperatorStore _operatorStore,
184:     IJBProjects _projects,
185:     IJBFundingCycleStore _fundingCycleStore,
186:     address _owner
187:   ) JBOperatable(_operatorStore) {
188:     projects = _projects;
189:     fundingCycleStore = _fundingCycleStore;

JBController.sol

369:   constructor(
370:     IJBOperatorStore _operatorStore,
371:     IJBProjects _projects,
372:     IJBDirectory _directory,
373:     IJBFundingCycleStore _fundingCycleStore,
374:     IJBTokenStore _tokenStore,
375:     IJBSplitsStore _splitsStore
376:   ) JBOperatable(_operatorStore) {
377:     projects = _projects;
378:     directory = _directory;
379:     fundingCycleStore = _fundingCycleStore;
380:     tokenStore = _tokenStore;
381:     splitsStore = _splitsStore;
382:   }

JBPayoutRedemptionPaymentTerminal.sol

283:   constructor(
284:     // payable constructor save the gas used to check msg.value==0
285:     address _token,
286:     uint256 _decimals,
287:     uint256 _currency,
288:     uint256 _baseWeightCurrency,
289:     uint256 _payoutSplitsGroup,
290:     IJBOperatorStore _operatorStore,
291:     IJBProjects _projects,
292:     IJBDirectory _directory,
293:     IJBSplitsStore _splitsStore,
294:     IJBPrices _prices,
295:     IJBSingleTokenPaymentTerminalStore _store,
296:     address _owner
297:   )
298:     payable
299:     JBSingleTokenPaymentTerminal(_token, _decimals, _currency)
300:     JBOperatable(_operatorStore)
301:   {
302:     baseWeightCurrency = _baseWeightCurrency;
303:     payoutSplitsGroup = _payoutSplitsGroup;
304:     projects = _projects;
305:     directory = _directory;
306:     splitsStore = _splitsStore;
307:     prices = _prices;
308:     store = _store;

JBSingleTokenPaymentTerminalStore.sol

276:   constructor(
277:     IJBDirectory _directory,
278:     IJBFundingCycleStore _fundingCycleStore,
279:     IJBPrices _prices
280:   ) {
281:     directory = _directory;
282:     fundingCycleStore = _fundingCycleStore;
283:     prices = _prices;
284:   }

TOOLS USED

Manual Analysis

Potential revert on transfer with IERC20

PROBLEM

Some popular tokens do not implement the ERC20 standard properly. For example Tether 's transfer() and transferFrom() functions do not return booleans as the specification requires: when these tokens are cast to IERC20, their function signatures do not match and the function calls revert. This means these tokens cannot be used by the protocol's payment terminal.

SEVERITY

Low

PROOF OF CONCEPT

JBERC20PaymentTerminal.sol

87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Use OpenZeppelin’s SafeERC20's safeTransfer()

Return value of transfer not checked

PROBLEM

Some ERC20 tokens do not revert upon failure in transfer()/transferFrom(), but simply return false. The function _transferFrom() in JBERC20PaymentTerminal.sol does not check the return value of IERC20.transfer and IERC20.transferFrom, meaning some token transfers may fail without getting noticed.

SEVERITY

Low

PROOF OF CONCEPT

JBERC20PaymentTerminal.sol

87:       ? IERC20(token).transfer(_to, _amount)
88:       : IERC20(token).transferFrom(_from, _to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

Check the return values of these function calls.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing Natspec function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Missing natspec comments include:

JBController.sol

258 @param _configuration

TOOLS USED

Manual Analysis

MITIGATION

Add a Natspec comment for this parameter

Return statements

PROBLEM

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

SEVERITY

Non-Critical

PROOF OF CONCEPT

JBController.sol

574:     return tokenStore.issueFor(_projectId, _name, _symbol);

TOOLS USED

Manual Analysis

MITIGATION

-574:     return tokenStore.issueFor(_projectId, _name, _symbol);
+574:     IJBToken token = tokenStore.issueFor(_projectId, _name, _symbol);

Return value of approve not checked

PROBLEM

Not all IERC20 implementations revert when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. When calling that function, it is hence recommended to check the return value.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBERC20PaymentTerminal.sol

99:     IERC20(token).approve(_to, _amount);

TOOLS USED

Manual Analysis

MITIGATION

-99:     IERC20(token).approve(_to, _amount);
+99:     (bool success) = IERC20(token).approve(_to, _amount);
+ require(success);

Consider also this issue that can happen with the .approve() method.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100_000) or exponentiation(10**5). Underscores are used throughout the contracts and do improve readability too, so this is more of a suggestion.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBPayoutRedemptionPaymentTerminal.sol

87:   uint256 internal constant _FEE_CAP = 50_000_000;

167:   uint256 public override fee = 25_000_000;

TOOLS USED

Manual Analysis

Timelock for critical parameter change

PROBLEM

It is good practice to add timelock to critical parameters changes, such as admin changes, to give users time to react. This is especially true for fee changes that directly affect protocol users. Merely a suggestion here as there is already a maximum value to which fee can be set, which can be deemed sufficient for users.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBPayoutRedemptionPaymentTerminal.sol

622:   function setFee(uint256 _fee) external virtual override onlyOwner {
623:     // The provided fee must be within the max.
624:     if (_fee > _FEE_CAP) revert FEE_TOO_HIGH();
625: 
626:     // Store the new fee.
627:     fee = _fee;

TOOLS USED

Manual Analysis

MITIGATION

Add a timelock to setFee

Transfer of ownership can be two-steps

PROBLEM

Consider using a 2-step ownership transfer for JBToken, so that the new owner must approve the ownership transfer, preventing issues if ownership was to be transferred to a random address.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

JBTokenStore.sol

266:       oldToken.transferOwnership(_projectId, _newOwner);

TOOLS USED

Manual Analysis

Gas Report

Table of Contents

• Array length should not be looked up in every iteration
• Caching storage variables in local variables to save gas
• Caching mapping accesses in local variables to save gas
• Calldata instead of memory for RO function parameters
• Clones for cheap contract deployment
• Comparison operators
• Constructor parameters should be avoided when possible
• Default value initialization
• Functions with access control cheaper if payable
• Immutable variables save storage
• Inline functions
• Mathematical optimizations
• Prefix increments
• unchecked arithmetic
• upgrade Solidity compiler version
• Variables can be private

Array length should not be looked up in every iteration

IMPACT

It wastes gas to read an array's length in every iteration of a for loop, even if it is a memory or calldata array: 3 gas per read.

PROOF OF CONCEPT

12 instances:

JBSplitsStore.sol

204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++)

211:       for (uint256 _j = 0; _j < _splits.length; _j++)

229:     for (uint256 _i = 0; _i < _splits.length; _i++)

JBOperatorStore.sol

85:     for (uint256 _i = 0; _i < _permissionIndexes.length; _i++)

135:     for (uint256 _i = 0; _i < _operatorData.length; _i++)

165:     for (uint256 _i = 0; _i < _indexes.length; _i++)

JBDirectory.sol

275:       for (uint256 _i; _i < _terminals.length; _i++)

276:         for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

JBController.sol

913:     for (uint256 _i = 0; _i < _splits.length; _i++)

1014:     for (uint256 _i; _i < _fundAccessConstraints.length; _i++)

JBPayoutRedemptionPaymentTerminal.sol

1008:     for (uint256 _i = 0; _i < _splits.length; )

JBSingleTokenPaymentTerminalStore.sol

862:     for (uint256 _i = 0; _i < _terminals.length; _i++)

TOOLS USED

Manual Analysis

MITIGATION

Caching the length in a variable before the for loop

Caching storage variables in local variables to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

1 instance:

JBPayoutRedemptionPaymentTerminal.sol

scope: _takeFeeFrom()

• fee is read twice in this block:

1199:       _heldFeesOf[_projectId].push(JBFee(_amount, uint32(fee), uint32(_feeDiscount), _beneficiary));
1200:
1201:       emit HoldFee(_projectId, _amount, fee, _feeDiscount, _beneficiary, msg.sender);

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables using local variables.

Caching mapping accesses in local variables to save gas

IMPACT

Anytime you are reading from a mapping value more than once, it is cheaper in gas cost to cache it, by saving one gkeccak256 operation - 30 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances:

JBDirectory.sol

139:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)

167:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)

TOOLS USED

Manual Analysis

MITIGATION

cache these mapping accesses using local variables.

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory,but it alleviates the compiler from the abi.decode() step that copies each index of the calldata to the memory index, each iteration costing 60 gas.

PROOF OF CONCEPT

20 instances:

JBFundingCycleStore.sol

scope: _initFor()

444:     JBFundingCycle memory _baseFundingCycle

scope: _mockFundingCycleBasedOn()

624:   function _mockFundingCycleBasedOn(JBFundingCycle memory _baseFundingCycle, bool _allowMidCycle)

scope: _deriveStartFrom()

664:   function _deriveStartFrom(JBFundingCycle memory _baseFundingCycle, uint256 _mustStartAtOrAfter)

scope: _deriveWeightFrom()

698:   function _deriveWeightFrom(JBFundingCycle memory _baseFundingCycle, uint256 _start)

scope: _deriveNumberFrom()

746:   function _deriveNumberFrom(JBFundingCycle memory _baseFundingCycle, uint256 _start)

scope: _isApproved()

770:   function _isApproved(uint256 _projectId, JBFundingCycle memory _fundingCycle)

JBController.sol

scope: launchProjectFor()

418:     IJBPaymentTerminal[] memory _terminals

scope: launchFundingCyclesFor()

470:     JBFundAccessConstraints[] memory _fundAccessConstraints,
471:     IJBPaymentTerminal[] memory _terminals,

scope: _configure()

988:     JBGroupedSplits[] memory _groupedSplits,
989:     JBFundAccessConstraints[] memory _fundAccessConstraints

JBPayoutRedemptionPaymentTerminal.sol

scope: redeemTokensOf()

393:     bytes memory _metadata

scope: _redeemTokensOf()

719:     bytes memory _metadata

scope: _takeFeeFrom()

1190:     JBFundingCycle memory _fundingCycle,

scope: _pay()

1268:     bytes memory _metadata

scope: _addToBalanceOf()

1359:     bytes memory _metadata

JBSingleTokenPaymentTerminalStore.sol

scope: recordPaymentFrom()

320:     bytes memory _metadata

scope: recordRedemptionFor()

419:     bytes memory _metadata

scope: _reclaimableOverflowDuring()

754:     JBFundingCycle memory _fundingCycle

scope: _overflowDuring()

807:     JBFundingCycle memory _fundingCycle

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Clones for cheap contract deployment

IMPACT

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

With the standard way using the new keyword, each contract created contains the entire logic. Using proxies allow only the first implementation to contain the logic, saving deployment costs on subsequent instances deployed.

PROOF OF CONCEPT

1 instance:

JBTokenStore.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Use a proxy system to deploy a project's JBToken implementations, see here for an example.

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

10 instances:

JBFundingCycleStore.sol

550:     if (block.timestamp >= _fundingCycle.start) return 0;

588:     if (
589:       _fundingCycle.duration > 0 && block.timestamp >= _fundingCycle.start + _fundingCycle.duration
590:     ) return 0

593:     if (block.timestamp >= _fundingCycle.start) return _fundingCycle.configuration;

600:     if (
601:       _baseFundingCycle.duration > 0 &&
602:       block.timestamp >= _baseFundingCycle.start + _baseFundingCycle.duration
603:     ) return 0;

676:     if (_nextImmediateStart >= _mustStartAtOrAfter) return _nextImmediateStart;

815:     else if (_ballotFundingCycle.ballot.duration() >= block.timestamp - _configuration)

JBSplitsStore.sol

206:       if (block.timestamp >= _currentSplits[_i].lockedUntil)

219:           _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil

JBController.sol

1075:     uint256 _unprocessedTokenBalanceOf = _processedTokenTracker >= 0

JBPayoutRedemptionPaymentTerminal.sol

1398:       else if (leftoverAmount >= _heldFees[_i].amount)

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-block.timestamp >= _fundingCycle.start;
+block.timestamp > _fundingCycle.start - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-block.timestamp >= _fundingCycle.start;
+block.timestamp > _fundingCycle.start;

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if variables/constants values are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

20 instances:

JBTokenStore.sol

160:   constructor(
161:     IJBOperatorStore _operatorStore,
162:     IJBProjects _projects,
163:     IJBDirectory _directory
164:   ) JBOperatable(_operatorStore) JBControllerUtility(_directory) {
165:     projects = _projects;
166:   }

JBSplitsStore.sol

120:   constructor(
121:     IJBOperatorStore _operatorStore,
122:     IJBProjects _projects,
123:     IJBDirectory _directory
124:   ) JBOperatable(_operatorStore) {
125:     projects = _projects;
126:     directory = _directory;
127:   }

JBDirectory.sol

182:   constructor(
183:     IJBOperatorStore _operatorStore,
184:     IJBProjects _projects,
185:     IJBFundingCycleStore _fundingCycleStore,
186:     address _owner
187:   ) JBOperatable(_operatorStore) {
188:     projects = _projects;
189:     fundingCycleStore = _fundingCycleStore;

JBController.sol

369:   constructor(
370:     IJBOperatorStore _operatorStore,
371:     IJBProjects _projects,
372:     IJBDirectory _directory,
373:     IJBFundingCycleStore _fundingCycleStore,
374:     IJBTokenStore _tokenStore,
375:     IJBSplitsStore _splitsStore
376:   ) JBOperatable(_operatorStore) {
377:     projects = _projects;
378:     directory = _directory;
379:     fundingCycleStore = _fundingCycleStore;
380:     tokenStore = _tokenStore;
381:     splitsStore = _splitsStore;
382:   }

JBPayoutRedemptionPaymentTerminal.sol

283:   constructor(
284:     // payable constructor save the gas used to check msg.value==0
285:     address _token,
286:     uint256 _decimals,
287:     uint256 _currency,
288:     uint256 _baseWeightCurrency,
289:     uint256 _payoutSplitsGroup,
290:     IJBOperatorStore _operatorStore,
291:     IJBProjects _projects,
292:     IJBDirectory _directory,
293:     IJBSplitsStore _splitsStore,
294:     IJBPrices _prices,
295:     IJBSingleTokenPaymentTerminalStore _store,
296:     address _owner
297:   )
298:     payable
299:     JBSingleTokenPaymentTerminal(_token, _decimals, _currency)
300:     JBOperatable(_operatorStore)
301:   {
302:     baseWeightCurrency = _baseWeightCurrency;
303:     payoutSplitsGroup = _payoutSplitsGroup;
304:     projects = _projects;
305:     directory = _directory;
306:     splitsStore = _splitsStore;
307:     prices = _prices;
308:     store = _store;

JBSingleTokenPaymentTerminalStore.sol

276:   constructor(
277:     IJBDirectory _directory,
278:     IJBFundingCycleStore _fundingCycleStore,
279:     IJBPrices _prices
280:   ) {
281:     directory = _directory;
282:     fundingCycleStore = _fundingCycleStore;
283:     prices = _prices;
284:   }

TOOLS USED

Manual Analysis

MITIGATION

Hardcode state variables or constants with their value instead of writing them during contract deployment with constructor parameters.

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes 3 gas.

PROOF OF CONCEPT

15 instances:

JBFundingCycleStore.sol

724:     for (uint256 i = 0; i < _discountMultiple; i++)

JBSplitsStore.sol

165:     for (uint256 _i = 0; _i < _groupedSplitsLength; )

204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++)

211:       for (uint256 _j = 0; _j < _splits.length; _j++)

227:     uint256 _percentTotal = 0

229:     for (uint256 _i = 0; _i < _splits.length; _i++)

304:     for (uint256 _i = 0; _i < _splitCount; _i++)

JBOperatorStore.sol

85:     for (uint256 _i = 0; _i < _permissionIndexes.length; _i++)

135:     for (uint256 _i = 0; _i < _operatorData.length; _i++)

165:     for (uint256 _i = 0; _i < _indexes.length; _i++)

JBController.sol

913:     for (uint256 _i = 0; _i < _splits.length; _i++)

JBPayoutRedemptionPaymentTerminal.sol

594:     for (uint256 _i = 0; _i < _heldFeeLength; )

1008:     for (uint256 _i = 0; _i < _splits.length; )

1396:     for (uint256 _i = 0; _i < _heldFeeLength; )

JBSingleTokenPaymentTerminalStore.sol

862:     for (uint256 _i = 0; _i < _terminals.length; _i++)

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Functions with access control cheaper if payable

PROBLEM

A function with access control marked as payable will be cheaper for legitimate callers: the compiler removes checks for msg.value, saving approximately 20 gas per function call.

PROOF OF CONCEPT

26 instances:

JBTokenStore.sol

188:   function issueFor(
189:     uint256 _projectId,
190:     string calldata _name,
191:     string calldata _symbol
192:   ) external override onlyController(_projectId) returns (IJBToken token)

236:   function changeFor(
237:     uint256 _projectId,
238:     IJBToken _token,
239:     address _newOwner
240:   ) external override onlyController(_projectId) returns (IJBToken oldToken)

283:   function mintFor(
284:     address _holder,
285:     uint256 _projectId,
286:     uint256 _amount,
287:     bool _preferClaimedTokens
288:   ) external override onlyController(_projectId)

320:   function burnFrom(
321:     address _holder,
322:     uint256 _projectId,
323:     uint256 _amount,
324:     bool _preferClaimedTokens
325:   ) external override onlyController(_projectId)

391:   function claimFor(
392:     address _holder,
393:     uint256 _projectId,
394:     uint256 _amount
395:   ) external override requirePermission(_holder, _projectId, JBOperations.CLAIM)

432:   function transferFrom(
433:     address _holder,
434:     uint256 _projectId,
435:     address _recipient,
436:     uint256 _amount
437:   ) external override requirePermission(_holder, _projectId, JBOperations.TRANSFER)

468:   function shouldRequireClaimingFor(uint256 _projectId, bool _flag)
469:     external
470:     override
471:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.REQUIRE_CLAIM)

JBFundingCycleStore.sol

299:   function configureFor(
300:     uint256 _projectId,
301:     JBFundingCycleData calldata _data,
302:     uint256 _metadata,
303:     uint256 _mustStartAtOrAfter
304:   ) external override onlyController(_projectId)

JBProjects.sol

162:   function setMetadataOf(uint256 _projectId, JBProjectMetadata calldata _metadata)
163:     external
164:     override
165:     requirePermission(ownerOf(_projectId), _projectId, JBOperations.SET_METADATA)

179:   function setTokenUriResolver(IJBTokenUriResolver _newResolver) external override onlyOwner

JBSplitsStore.sol

147:   function set(
148:     uint256 _projectId,
149:     uint256 _domain,
150:     JBGroupedSplits[] calldata _groupedSplits
151:   )
152:     external
153:     override
154:     requirePermissionAllowingOverride(
155:       projects.ownerOf(_projectId),
156:       _projectId,
157:       JBOperations.SET_SPLITS,
158:       address(directory.controllerOf(_projectId)) == msg.sender
159:     )

JBPrices.sol

109:   function addFeedFor(
110:     uint256 _currency,
111:     uint256 _base,
112:     IJBPriceFeed _feed
113:   ) external override onlyOwner

JBDirectory.sol

211:   function setControllerOf(uint256 _projectId, address _controller)
212:     external
213:     override
214:     requirePermissionAllowingOverride(
215:       projects.ownerOf(_projectId),
216:       _projectId,
217:       JBOperations.SET_CONTROLLER,
218:       (msg.sender == address(controllerOf[_projectId]) ||
219:         (isAllowedToSetFirstController[msg.sender] && controllerOf[_projectId] == address(0)))
220:     )

251:   function setTerminalsOf(uint256 _projectId, IJBPaymentTerminal[] calldata _terminals)
252:     external
253:     override
254:     requirePermissionAllowingOverride(
255:       projects.ownerOf(_projectId),
256:       _projectId,
257:       JBOperations.SET_TERMINALS,
258:       msg.sender == address(controllerOf[_projectId])
259:     )

297:   function setPrimaryTerminalOf(
298:     uint256 _projectId,
299:     address _token,
300:     IJBPaymentTerminal _terminal
301:   )
302:     external
303:     override
304:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.SET_PRIMARY_TERMINAL)

333:   function setIsAllowedToSetFirstController(address _address, bool _flag)
334:     external
335:     override
336:     onlyOwner

JBController.sol

464:   function launchFundingCyclesFor(
465:     uint256 _projectId,
466:     JBFundingCycleData calldata _data,
467:     JBFundingCycleMetadata calldata _metadata,
468:     uint256 _mustStartAtOrAfter,
469:     JBGroupedSplits[] calldata _groupedSplits,
470:     JBFundAccessConstraints[] memory _fundAccessConstraints,
471:     IJBPaymentTerminal[] memory _terminals,
472:     string memory _memo
473:   )
474:     external
475:     virtual
476:     override
477:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.RECONFIGURE)

520:   function reconfigureFundingCyclesOf(
521:     uint256 _projectId,
522:     JBFundingCycleData calldata _data,
523:     JBFundingCycleMetadata calldata _metadata,
524:     uint256 _mustStartAtOrAfter,
525:     JBGroupedSplits[] calldata _groupedSplits,
526:     JBFundAccessConstraints[] calldata _fundAccessConstraints,
527:     string calldata _memo
528:   )
529:     external
530:     virtual
531:     override
532:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.RECONFIGURE)

562:   function issueTokenFor(
563:     uint256 _projectId,
564:     string calldata _name,
565:     string calldata _symbol
566:   )
567:     external
568:     virtual
569:     override
570:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.ISSUE)

588:   function changeTokenOf(
589:     uint256 _projectId,
590:     IJBToken _token,
591:     address _newOwner
592:   )
593:     external
594:     virtual
595:     override
596:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.CHANGE_TOKEN)

711:   function burnTokensOf(
712:     address _holder,
713:     uint256 _projectId,
714:     uint256 _tokenCount,
715:     string calldata _memo,
716:     bool _preferClaimedTokens
717:   )
718:     external
719:     virtual
720:     override
721:     requirePermissionAllowingOverride(
722:       _holder,
723:       _projectId,
724:       JBOperations.BURN,
725:       directory.isTerminalOf(_projectId, IJBPaymentTerminal(msg.sender))
726:     )

800:   function migrate(uint256 _projectId, IJBMigratable _to)
801:     external
802:     virtual
803:     override
804:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.MIGRATE_CONTROLLER)

JBPayoutRedemptionPaymentTerminal.sol

385:   function redeemTokensOf(
386:     address _holder,
387:     uint256 _projectId,
388:     uint256 _tokenCount,
389:     address _token,
390:     uint256 _minReturnedTokens,
391:     address payable _beneficiary,
392:     string memory _memo,
393:     bytes memory _metadata
394:   )
395:     external
396:     virtual
397:     override
398:     requirePermission(_holder, _projectId, JBOperations.REDEEM)

470:   function useAllowanceOf(
471:     uint256 _projectId,
472:     uint256 _amount,
473:     uint256 _currency,
474:     address _token,
475:     uint256 _minReturnedTokens,
476:     address payable _beneficiary,
477:     string memory _memo
478:   )
479:     external
480:     virtual
481:     override
482:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.USE_ALLOWANCE)

502:   function migrate(uint256 _projectId, IJBPaymentTerminal _to)
503:     external
504:     virtual
505:     override
506:     requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.MIGRATE_TERMINAL)

573:   function processFees(uint256 _projectId)
574:     external
575:     virtual
576:     override
577:     requirePermissionAllowingOverride(
578:       projects.ownerOf(_projectId),
579:       _projectId,
580:       JBOperations.PROCESS_FEES,
581:       msg.sender == owner()
582:     )

TOOLS USED

Manual Analysis

MITIGATION

Add the payable modifier to these functions.

Inline functions

PROBLEM

When we define private or internal functions to perform computation:

• The contract’s code size gets bigger
• the function call consumes more gas than executing it as an inlined function (part of the code, without the function call)

Inlining functions that are only called once saves gas. As it can affect readability, this is merely a suggestion for the team.

PROOF OF CONCEPT

5 instances:

JBFundingCycleStore.sol

385:   function _configureIntrinsicPropertiesFor(
386:     uint256 _projectId,
387:     uint256 _configuration,
388:     uint256 _weight,
389:     uint256 _mustStartAtOrAfter
390:   ) private

542: function _standbyOf(uint256 _projectId) private view returns (uint256 configuration)

JBSplitsStore.sol

194:   function _set(
195:     uint256 _projectId,
196:     uint256 _domain,
197:     uint256 _group,
198:     JBSplit[] memory _splits
199:   ) internal

JBDirectory.sol

355:   function _addTerminalIfNeeded(uint256 _projectId, IJBPaymentTerminal _terminal) private

JBController.sol

900:   function _distributeToReservedTokenSplitsOf(
901:     uint256 _projectId,
902:     uint256 _domain,
903:     uint256 _group,
904:     uint256 _amount
905:   ) internal returns (uint256 leftoverAmount)

TOOLS USED

Manual Analysis

MITIGATION

Consider inlining these functions where they are called

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

6 instances include:

JBPayoutRedemptionPaymentTerminal.sol

860:         _feeEligibleDistributionAmount += _leftoverDistributionAmount;

1038:             feeEligibleDistributionAmount += _payoutAmount

1103:               feeEligibleDistributionAmount += _payoutAmount;

1145:           feeEligibleDistributionAmount += _payoutAmount;

1401:           refundedFees += _feeAmount(
1402:             _heldFees[_i].amount,
1403:             _heldFees[_i].fee,
1404:             _heldFees[_i].feeDiscount
1405:           );

1417:           refundedFees += _feeAmount(leftoverAmount, _heldFees[_i].fee, _heldFees[_i].feeDiscount);

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops.

PROOF OF CONCEPT

15 instances:

JBFundingCycleStore.sol

724:     for (uint256 i = 0; i < _discountMultiple; i++)

JBSplitsStore.sol

204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++)

211:       for (uint256 _j = 0; _j < _splits.length; _j++)

229:     for (uint256 _i = 0; _i < _splits.length; _i++)

304:     for (uint256 _i = 0; _i < _splitCount; _i++)

JBOperatorStore.sol

85:     for (uint256 _i = 0; _i < _permissionIndexes.length; _i++)

135:     for (uint256 _i = 0; _i < _operatorData.length; _i++)

165:     for (uint256 _i = 0; _i < _indexes.length; _i++)

JBDirectory.sol

139:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)

167:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)

275:       for (uint256 _i; _i < _terminals.length; _i++)

276:         for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

JBController.sol

913:     for (uint256 _i = 0; _i < _splits.length; _i++)

1014:     for (uint256 _i; _i < _fundAccessConstraints.length; _i++)

JBSingleTokenPaymentTerminalStore.sol

862:     for (uint256 _i = 0; _i < _terminals.length; _i++)

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

20 instances:

JBTokenStore.sol

350:     else _claimedTokensToBurn = _unclaimedBalance < _amount ? _amount - _unclaimedBalance : 0; //@audit cannot underflow because of the condition _unclaimedBalance < _amount

409:     unclaimedBalanceOf[_holder][_projectId] = unclaimedBalanceOf[_holder][_projectId] - _amount; //@audit cannot underflow because of the check line 406

448:     unclaimedBalanceOf[_holder][_projectId] = unclaimedBalanceOf[_holder][_projectId] - _amount; //@audit cannot underflow because of the check line 445

JBFundingCycleStore.sol

679:     uint256 _timeFromImmediateStartMultiple = (_mustStartAtOrAfter - _nextImmediateStart) //@audit cannot underflow because of the check line 676

724:     for (uint256 i = 0; i < _discountMultiple; i++) //@audit i cannot overflow because it is a for loop

JBSplitsStore.sol

204:     for (uint256 _i = 0; _i < _currentSplits.length; _i++) //@audit _i cannot overflow because it is a for loop

211:       for (uint256 _j = 0; _j < _splits.length; _j++)  //@audit _j cannot overflow because it is a for loop

229:     for (uint256 _i = 0; _i < _splits.length; _i++) //@audit _i cannot overflow because it is a for loop

304:     for (uint256 _i = 0; _i < _splitCount; _i++) //@audit _i cannot overflow because it is a for loop

JBOperatorStore.sol

85:     for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) //@audit _i cannot overflow because it is a for loop

135:     for (uint256 _i = 0; _i < _operatorData.length; _i++) //@audit _i cannot overflow because it is a for loop

165:     for (uint256 _i = 0; _i < _indexes.length; _i++) //@audit _i cannot overflow because it is a for loop

JBDirectory.sol

139:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) //@audit _i cannot overflow because it is a for loop

167:     for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) //@audit _i cannot overflow because it is a for loop

275:       for (uint256 _i; _i < _terminals.length; _i++) //@audit _i cannot overflow because it is a for loop

276:         for (uint256 _j = _i + 1; _j < _terminals.length; _j++) //@audit _j cannot overflow because it is a for loop

JBController.sol

913:     for (uint256 _i = 0; _i < _splits.length; _i++) //@audit _i cannot overflow because it is a for loop

1014:     for (uint256 _i; _i < _fundAccessConstraints.length; _i++) //@audit _i cannot overflow because it is a for loop

JBSingleTokenPaymentTerminalStore.sol

834:     return _balanceOf > _distributionLimitRemaining ? _balanceOf - _distributionLimitRemaining : 0; //@audit _balanceOf - _distributionLimitRemaining cannot underflow because of the condition _balanceOf > _distributionLimitRemaining

862:     for (uint256 _i = 0; _i < _terminals.length; _i++) //@audit _i cannot overflow because it is a for loop

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Upgrade Solidity compiler version

IMPACT

• 0.8.0 introduces built-in overflow/underflow checks, removing the need to use external libraries to perform these checks.
• 0.8.2 introduces automatic inlining
• 0.8.3 introduces cheaper read operations on multiple storage reads.
• 0.8.4 introduces custom errors, cheaper upon deployment than require statements.
• 0.8.10 removes contract existence checks if the external call has a return value - 700 gas

PROOF OF CONCEPT

Instances:

JBTokenStore.sol

2: pragma solidity 0.8.6;

JBFundingCycleStore.sol

2: pragma solidity 0.8.6;

JBProjects.sol

2: pragma solidity 0.8.6;

JBSplitsStore.sol

2: pragma solidity 0.8.6;

JBPrices.sol

2: pragma solidity 0.8.6;

JBOperatorStore.sol

2: pragma solidity 0.8.6;

JBDirectory.sol

2: pragma solidity 0.8.6;

JBController.sol

2: pragma solidity 0.8.6;

JBPayoutRedemptionPaymentTerminal.sol

2: pragma solidity 0.8.6;

JBSingleTokenPaymentTerminalStore.sol

2: pragma solidity 0.8.6;

JBETHPaymentTerminal.sol

2: pragma solidity 0.8.6;

JBERC20PaymentTerminal.sol

2: pragma solidity 0.8.6;

TOOLS USED

Manual Analysis

MITIGATION

Upgrade these contracts compiler versions.

Variables can be private

IMPACT

Marking variables as private save gas upon deployment, as the compiler does not have to create getter functions for these variables. It is worth noting that a private variable can still be read using either the verified contract source code or the bytecode. Another method could be to record the variable's new value off-chain - easy to do when an event is emitted upon change.

PROOF OF CONCEPT

1 instance:

JBProjects.sol

40:   uint256 public override count = 0;

TOOLS USED

Manual Analysis

MITIGATION

Make count private instead of public