Juicebox V2 contest - MiloTruck's results

Non-Critical Issues

.code.length can be used instead of extcodesize()

Since Solidity v0.8.6 is used in JBFundingCycleStore.sol, .code.length can be used to check if an address is a contract containing code, as seen in the docs.

Consider changing lines 317-322 in JBFundingCycleStore.sol from:

      address _ballot = address(_data.ballot);
      uint32 _size;
      assembly {
        _size := extcodesize(_ballot) // No contract at the address ?
      }
      if (_size == 0) revert INVALID_BALLOT();

to:

      address _ballot = address(_data.ballot);
      if (_ballot.code.length == 0) revert INVALID_BALLOT();

This would help improve code readability.

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/JBFundingCycleStore.sol:
 162:        if (fundingCycle.start > block.timestamp)
 230:        if (!_isApproved(_projectId, _fundingCycle) || block.timestamp < _fundingCycle.start)
 332:        uint256 _configuration = block.timestamp;
 340:        _mustStartAtOrAfter > block.timestamp ? _mustStartAtOrAfter : block.timestamp
 408:        if (!_isApproved(_projectId, _baseFundingCycle) || block.timestamp < _baseFundingCycle.start)
 550:        if (block.timestamp >= _fundingCycle.start) return 0;
 561:        block.timestamp < _fundingCycle.start - _baseFundingCycle.duration
 589:        _fundingCycle.duration > 0 && block.timestamp >= _fundingCycle.start + _fundingCycle.duration
 593:        if (block.timestamp >= _fundingCycle.start) return _fundingCycle.configuration;
 602:        block.timestamp >= _baseFundingCycle.start + _baseFundingCycle.duration
 632:        ? block.timestamp + 1
 633:        : block.timestamp - _baseFundingCycle.duration + 1;
 815:        else if (_ballotFundingCycle.ballot.duration() >= block.timestamp - _configuration)

contracts/JBSplitsStore.sol:
 206:        if (block.timestamp >= _currentSplits[_i].lockedUntil) continue;

Gas Optimizations Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/JBFundingCycleStore.sol:
 724:        for (uint256 i = 0; i < _discountMultiple; i++) {

contracts/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++) {
 229:        for (uint256 _i = 0; _i < _splits.length; _i++) {
 304:        for (uint256 _i = 0; _i < _splitCount; _i++) {

contracts/JBSingleTokenPaymentTerminalStore.sol:
 862:        for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/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++) {

contracts/JBController.sol:
 913:        for (uint256 _i = 0; _i < _splits.length; _i++) {

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
 594:        for (uint256 _i = 0; _i < _heldFeeLength; ) {
1008:        for (uint256 _i = 0; _i < _splits.length; ) {
1396:        for (uint256 _i = 0; _i < _heldFeesLength; ) {

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

contracts/JBDirectory.sol:
 275:        for (uint256 _i; _i < _terminals.length; _i++)
 276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

contracts/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++) {

contracts/JBSingleTokenPaymentTerminalStore.sol:
 862:        for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/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++) {

contracts/JBController.sol:
 913:        for (uint256 _i = 0; _i < _splits.length; _i++) {
1014:        for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
1008:        for (uint256 _i = 0; _i < _splits.length; ) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

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

can be changed to:

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

Consider making the following change to these lines:

contracts/JBFundingCycleStore.sol:
 724:        for (uint256 i = 0; i < _discountMultiple; i++) {

contracts/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++)

contracts/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++) {

contracts/JBSingleTokenPaymentTerminalStore.sol:
 862:        for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/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++) {

contracts/JBController.sol:
 913:        for (uint256 _i = 0; _i < _splits.length; _i++) {
1014:        for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/JBFundingCycleStore.sol:
 724:        for (uint256 i = 0; i < _discountMultiple; i++) {

contracts/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++)

contracts/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++) {

contracts/JBSingleTokenPaymentTerminalStore.sol:
 862:        for (uint256 _i = 0; _i < _terminals.length; _i++)

contracts/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++) {

contracts/JBController.sol:
 913:        for (uint256 _i = 0; _i < _splits.length; _i++) {
1014:        for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/JBFundingCycleStore.sol:
 149:        if (_standbyFundingCycleConfiguration > 0) {
 210:        if (_fundingCycleConfiguration > 0) {
 347:        _data.duration > 0 ||
 348:        _data.discountRate > 0
 364:        if (_metadata > 0) _metadataOf[_projectId][_configuration] = _metadata;
 469:        _weight = _weight > 0
 560:        _baseFundingCycle.duration > 0 &&
 589:        _fundingCycle.duration > 0 && block.timestamp >= _fundingCycle.start + _fundingCycle.duration
 601:        _baseFundingCycle.duration > 0 &&

contracts/JBSplitsStore.sol:
 259:        if (_splits[_i].lockedUntil > 0 || _splits[_i].allocator != IJBSplitAllocator(address(0))) {
 272:        } else if (_packedSplitParts2Of[_projectId][_domain][_group][_i] > 0)
 326:        if (_packedSplitPart2 > 0) {

contracts/JBTokenStore.sol:
 356:        if (_unclaimedTokensToBurn > 0) {
 367:        if (_claimedTokensToBurn > 0) _token.burn(_projectId, _holder, _claimedTokensToBurn);

contracts/JBSingleTokenPaymentTerminalStore.sol:
 475:        if (_currentOverflow > 0)
 518:        if (reclaimAmount > 0)

contracts/JBController.sol:
 438:        if (_terminals.length > 0) directory.setTerminalsOf(projectId, _terminals);
 481:        if (fundingCycleStore.latestConfigurationOf(_projectId) > 0)
 498:        if (_terminals.length > 0) directory.setTerminalsOf(_projectId, _terminals);
 875:        if (_leftoverTokenCount > 0) tokenStore.mintFor(_owner, _projectId, _leftoverTokenCount, false);
 925:        if (_tokenCount > 0) {
1032:        if (_constraints.distributionLimit > 0)
1040:        if (_constraints.overflowAllowance > 0)

contracts/JBProjects.sol:
 143:        if (bytes(_metadata.content).length > 0)

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
 346:        if (msg.value > 0) revert NO_MSG_VALUE_ALLOWED();
 516:        if (balance > 0) {
 552:        if (msg.value > 0) revert NO_MSG_VALUE_ALLOWED();
 745:        if (_tokenCount > 0)
 772:        if (reclaimAmount > 0) _transferFrom(address(this), _beneficiary, reclaimAmount);
 884:        if (netLeftoverDistributionAmount > 0)
 964:        if (netDistributedAmount > 0)
1022:        if (_payoutAmount > 0) {
1297:        if (_tokenCount > 0)

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/JBSplitsStore.sol:
 209:        bool _includesLocked = false;
 227:        uint256 _percentTotal = 0;

contracts/JBProjects.sol:
  40:        uint256 public override count = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables and directly using the expressions on the right can help to reduce gas cost and contract size.

Instances include:

contracts/JBFundingCycleStore.sol:
 631:        uint256 _mustStartAtOrAfter = !_allowMidCycle
 632:          ? block.timestamp + 1
 633:          : block.timestamp - _baseFundingCycle.duration + 1;

 679:        uint256 _timeFromImmediateStartMultiple = (_mustStartAtOrAfter - _nextImmediateStart) %
 680:          _baseFundingCycle.duration;

 719:        uint256 _startDistance = _start - _baseFundingCycle.start;
 722:        uint256 _discountMultiple = _startDistance / _baseFundingCycle.duration;
 755:        uint256 _startDistance = _start - _baseFundingCycle.start;

contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:
 521:        uint256 _payableValue = token == JBTokens.ETH ? balance : 0;
1229:        uint256 _payableValue = token == JBTokens.ETH ? _amount : 0;