Juicebox V2 contest - TomJ's results

[L-01] Immutable addresses should 0-Check

Issue

I recommend adding check of 0-address for immutable addresses. Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

PoC

2 immutable addresses of "projects" and "directory" of JBSplitsStore.sol https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L103-L109

2 immutable addresses of "projects" and "fundingCycleStore" of JBDirectory.sol https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L182-L192

4 immutable addresses of "projects", "fundingCycleStore", "tokenStore" "splitsStore" and "directory" of JBController.sol https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L369-L382

4 immutable addresses of "projects", "directory" "splitsStore", "prices" and "store" of JBPayoutRedemptionPaymentTerminal.sol https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L283-L297

3 immutable addresses of "directory", "fundingCycleStore" and "prices" of JBSingleTokenPaymentTerminalStore.sol https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSingleTokenPaymentTerminalStore.sol#L276-L284

Mitigation

Add 0-address check for immutable addresses shown in above PoC.

Table of Contents

• Should Use Unchecked Block where Over/Underflow is not Possible
• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Unnecessary Default Value Initialization
• Store Array's Length as a Variable
• ++i Costs Less Gas than i++
• Use Calldata instead of Memory for Read Only Function Parameters

 

[G-01] Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

Issue found at

1. JBSplitsStore.sol

Wrap line 237 with unchecked since underflow is not possible due to line 234 check
234:      if (_splits[_i].projectId > type(uint56).max) revert INVALID_PROJECT_ID();
237:      _percentTotal = _percentTotal + _splits[_i].percent;

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L227-L237

Mitigation

Wrap the code with uncheck like described in above PoC.

 

[G-02] Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

1. JBPayoutRedemptionPaymentTerminal.sol

• Cache _PROTOCOL_PROJECT_ID of function _processFee 3 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1219-L1233

2. JBController.sol

• Cache _processedTokenTrackerOf of function mintTokensOf 4 SLOADs to 1 SLOAD, 1 MSTORE and 3 MLOAD https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L666-L680

• Cache directory of function launchFundingCyclesFor 2 SLOADs to 1 SLOAD, 1 MSTORE and 1 MLOAD https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L485-L498

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

[G-03] Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

Issue found at

1. JBFundingCycleStore.sol

• Remove "_fundingCycleConfiguration" variable of latestConfiguredOf function https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L111 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L114 Mitigation: Delete line 111 and replace line 114 like shown below

    fundingCycle = _getStructFor(_projectId, latestConfigurationOf[_projectId]);

• Remove "_fundingCycleConfiguration" variable of latestConfiguredOf function https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L257 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L260 Mitigation: Delete line 257 and replace line 260 like shown below

    JBFundingCycle memory _fundingCycle = _getStructFor(_projectId, latestConfigurationOf[_projectId]);

• Remove "_ballot" variable of configureFor function https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L317 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L320 Mitigation: Delete line 317 and replace line 320 like shown below

        _size := extcodesize(address(_data.ballot)) // No contract at the address ?

• Remove "_startDistance" variable of _deriveWeightFrom function https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L719 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L722 Mitigation: Delete line 719 and replace line 722 like shown below

    uint256 _discountMultiple = (_start - _baseFundingCycle.start) / _baseFundingCycle.duration;

• Remove "_startDistance" variable of _deriveNumberFrom function https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L755 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L758 Mitigation: Delete line 755 and replace line 758 like shown below

    return _baseFundingCycle.number + ((_start - _baseFundingCycle.start) / _baseFundingCycle.duration);

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

[G-04] Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

./JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
./JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
./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++) {
./JBTokenStore.sol:262:    if (oldToken != IJBToken(address(0))) projectOf[oldToken] = 0;
./JBTokenStore.sol:345:      _claimedTokensToBurn = 0;
./JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
./JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _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++) {
./JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
./JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
./JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
./JBPayoutRedemptionPaymentTerminal.sol:1419:        leftoverAmount = 0;
./JBPayoutRedemptionPaymentTerminal.sol:1469:      feeDiscount = 0;
./JBPayoutRedemptionPaymentTerminal.sol:1475:        feeDiscount = 0;
./JBPayoutRedemptionPaymentTerminal.sol:1479:    if (feeDiscount > JBConstants.MAX_FEE_DISCOUNT) feeDiscount = 0;
./JBSingleTokenPaymentTerminalStore.sol:730:    balanceOf[IJBSingleTokenPaymentTerminal(msg.sender)][_projectId] = 0;
./JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
./JBProjects.sol:40:  uint256 public override count = 0;
./JBSplitsStore.sol:209:      bool _includesLocked = false;

Mitigation

I suggest removing default value initialization. For example,

• uint startingAllowance;
• for (uint i; i < proposal.targets.length; i++) {

 

[G-05] Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

./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++) {
./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++)
./JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
./JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _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++) {
./JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
./JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)

Mitigation

Store array's length as a variable before looping it. For example, I suggest changing it to

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

 

[G-06] ++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

./JBSplitsStore.sol:173:        ++_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++) {
./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++) {
./JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
./JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _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++)

Mitigation

Change it to postfix increments/decrements. For example,

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

 

[G-07] Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./JBFundingCycleStore.sol:624:  function _mockFundingCycleBasedOn(JBFundingCycle memory _baseFundingCycle, bool _allowMidCycle)
./JBFundingCycleStore.sol:664:  function _deriveStartFrom(JBFundingCycle memory _baseFundingCycle, uint256 _mustStartAtOrAfter)
./JBFundingCycleStore.sol:698:  function _deriveWeightFrom(JBFundingCycle memory _baseFundingCycle, uint256 _start)
./JBFundingCycleStore.sol:746:  function _deriveNumberFrom(JBFundingCycle memory _baseFundingCycle, uint256 _start)
./JBFundingCycleStore.sol:770:  function _isApproved(uint256 _projectId, JBFundingCycle memory _fundingCycle)
./JBController.sol:418:    IJBPaymentTerminal[] memory _terminals,
./JBController.sol:419:    string memory _memo
./JBController.sol:470:    JBFundAccessConstraints[] memory _fundAccessConstraints,
./JBController.sol:471:    IJBPaymentTerminal[] memory _terminals,
./JBController.sol:472:    string memory _memo
./JBController.sol:841:  function _distributeReservedTokensOf(uint256 _projectId, string memory _memo)
./JBController.sol:988:    JBGroupedSplits[] memory _groupedSplits,
./JBController.sol:989:    JBFundAccessConstraints[] memory _fundAccessConstraints
./JBPayoutRedemptionPaymentTerminal.sol:392:    string memory _memo,
./JBPayoutRedemptionPaymentTerminal.sol:393:    bytes memory _metadata
./JBPayoutRedemptionPaymentTerminal.sol:477:    string memory _memo
./JBPayoutRedemptionPaymentTerminal.sol:718:    string memory _memo,
./JBPayoutRedemptionPaymentTerminal.sol:719:    bytes memory _metadata
./JBPayoutRedemptionPaymentTerminal.sol:927:    string memory _memo
./JBPayoutRedemptionPaymentTerminal.sol:1190:    JBFundingCycle memory _fundingCycle,
./JBPayoutRedemptionPaymentTerminal.sol:1267:    string memory _memo,
./JBPayoutRedemptionPaymentTerminal.sol:1268:    bytes memory _metadata
./JBPayoutRedemptionPaymentTerminal.sol:1358:    string memory _memo,
./JBPayoutRedemptionPaymentTerminal.sol:1359:    bytes memory _metadata
./JBSingleTokenPaymentTerminalStore.sol:320:    bytes memory _metadata
./JBSingleTokenPaymentTerminalStore.sol:326:      JBFundingCycle memory fundingCycle,
./JBSingleTokenPaymentTerminalStore.sol:329:      string memory memo
./JBSingleTokenPaymentTerminalStore.sol:418:    string memory _memo,
./JBSingleTokenPaymentTerminalStore.sol:419:    bytes memory _metadata
./JBSingleTokenPaymentTerminalStore.sol:425:      JBFundingCycle memory fundingCycle,
./JBSingleTokenPaymentTerminalStore.sol:428:      string memory memo
./JBSingleTokenPaymentTerminalStore.sol:754:    JBFundingCycle memory _fundingCycle,
./JBSingleTokenPaymentTerminalStore.sol:807:    JBFundingCycle memory _fundingCycle,

Mitigation

Change memory to calldata