Juicebox V2 contest - m_Rassska's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L332-L365

Vulnerability details

Impact

.transfer or .transferFrom doesn't revert in case of failure, therefore some unexpected behavior happens.

Proof of Concept

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L332-L365

Tools Used

• Slither

Recommended Mitigation Steps

• Maybe, a simple require to cover that?

Table of contents

• [L-01] Out of boundaries of terminals.length
• [L-02] Unchecked .transfer, .transferFrom and .approve methods return value

[L-01] Out of boundaries of terminals.length<a name="L-01"></a>

Description:

• In a nested loop _j = i + 1, but what if terminals.length - 1? In this case _j = terminals.length which is out of the boundaries.

• Fortunately, it has no a massive impact, but if those vars equals to each other, extra error will be reverted.

All occurances:

• Contracts:

    file: contracts/JBDirectory.sol
    ...............................
    
      // Lines: [274-277]
      if (_terminals.length > 1)
        for (uint256 _i; _i < _terminals.length; _i++)
          for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
            if (_terminals[_i] == _terminals[_j]) revert DUPLICATE_TERMINALS();

Recommendations:

• Nested loop should go till _j < terminals.length - 1;.

[L-02] Unchecked return value of .transfer, .transferFrom and .approve OZ methods<a name="L-02"></a>

Description:

• Following methods return bool value which represent the status. False - something happend, true - okay.

All occurances:

• Contracts:

    file: contracts/JBERC20PaymentTerminal.sol
    ...............................
    
      // Lines: [86-88]
      _from == address(this)
        ? IERC20(token).transfer(_to, _amount)
        : IERC20(token).transferFrom(_from, _to, _amount);

Recommendations:

• Write simple require to check the value of these methods with corresponding behavior.

Table of contents

• [0x0] Disclaimer
• [G-01] Try ++i instead of i++
• [G-02] Try uint256 _i; instead of uint256 _i = 0;
• [G-03] Rename frequently invoked functions
• [G-04] Consider a = a + b instead of a += b
• [G-05] Consider marking onlyOwner functions as payable
• [G-06] Use constants instead of type(uint232).max

Disclaimer<a name="0x0"></a>

• Please, consider everything described below as a general recommendation. These notes will represent potential possibilities to optimize gas consumption. It's okay, if something is not suitable in your case. Just let me know the reason in the comments section. Enjoy!

[G-01] Try ++i instead of i++<a name="G-01"></a>

Description:

• In case of i++, the compiler needs to create a temp variable to return and then it gets incremented.
• In case of ++i, the compiler just simply returns already incremented value.

Recommendations:

• Use prefix increment instead of postfix.

All occurances:

• Contracts:

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

**[G-02] Try unchecked{++i}; instead of i++; in loops **<a name="G-02"></a>

Description:

• If the for loop runs 100 times, it's about 10k units of gas which can be saved in comparison. Don't worry about overflow, when the number is just simply getting incremented by 1. There are ~1e80 atoms in the universe, so 2^256 is closed to that number, therefore it's no a way to be overflowed, because of the gas limit as well.

Recommendations:

• Try to use unchecked{} box where it's no a way to get a overflow/underflow. Significant gas usage optimization.

All occurances:

• Contracts:

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

**[G-03] Rename frequently invoked functions **<a name="G-03"></a>

Description:

• Well, the compiler will sort all functions by their id.
• After doing so, our contracts/JBProjects.sol selector's structure looks like that: JBProjects:

  +--------------------------------------------------------------+------------+
  |                             Name                             |     ID     |
  +--------------------------------------------------------------+------------+
  |                           owner()                            | 0x8da5cb5b |
  |                     renounceOwnership()                      | 0x715018a6 |
  |                  transferOwnership(address)                  | 0xf2fde38b |
  |                      getVotes(address)                       | 0x9ab24eb0 |
  |                getPastVotes(address,uint256)                 | 0x3a46b1a8 |
  |                 getPastTotalSupply(uint256)                  | 0x8e539e8c |
  |                      delegates(address)                      | 0x587cde1e |
  |                      delegate(address)                       | 0x5c19a95c |
  | delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32) | 0xc3cda520 |
  |                       nonces(address)                        | 0x7ecebe00 |
  |                      DOMAIN_SEPARATOR()                      | 0x3644e515 |
  |                      getVotes(address)                       | 0x9ab24eb0 |
  |                getPastVotes(address,uint256)                 | 0x3a46b1a8 |
  |                 getPastTotalSupply(uint256)                  | 0x8e539e8c |
  |                      delegates(address)                      | 0x587cde1e |
  |                      delegate(address)                       | 0x5c19a95c |
  | delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32) | 0xc3cda520 |
  |                  constructor(string,string)                  | 0xd4d8c5c3 |
  |                  supportsInterface(bytes4)                   | 0x01ffc9a7 |
  |                      balanceOf(address)                      | 0x70a08231 |
  |                       ownerOf(uint256)                       | 0x6352211e |
  |                            name()                            | 0x06fdde03 |
  |                           symbol()                           | 0x95d89b41 |
  |                      tokenURI(uint256)                       | 0xc87b56dd |
  |                   approve(address,uint256)                   | 0x095ea7b3 |
  |                     getApproved(uint256)                     | 0x081812fc |
  |               setApprovalForAll(address,bool)                | 0xa22cb465 |
  |              isApprovedForAll(address,address)               | 0xe985e9c5 |
  |            transferFrom(address,address,uint256)             | 0x23b872dd |
  |          safeTransferFrom(address,address,uint256)           | 0x42842e0e |
  |       safeTransferFrom(address,address,uint256,bytes)        | 0xb88d4fde |
  |                            name()                            | 0x06fdde03 |
  |                           symbol()                           | 0x95d89b41 |
  |                      tokenURI(uint256)                       | 0xc87b56dd |
  |                      balanceOf(address)                      | 0x70a08231 |
  |                       ownerOf(uint256)                       | 0x6352211e |
  |          safeTransferFrom(address,address,uint256)           | 0x42842e0e |
  |            transferFrom(address,address,uint256)             | 0x23b872dd |
  |                   approve(address,uint256)                   | 0x095ea7b3 |
  |                     getApproved(uint256)                     | 0x081812fc |
  |               setApprovalForAll(address,bool)                | 0xa22cb465 |
  |              isApprovedForAll(address,address)               | 0xe985e9c5 |
  |       safeTransferFrom(address,address,uint256,bytes)        | 0xb88d4fde |
  |                  supportsInterface(bytes4)                   | 0x01ffc9a7 |
  |                  supportsInterface(bytes4)                   | 0x01ffc9a7 |
  |                       operatorStore()                        | 0xad007d63 |
  |                           count()                            | 0x06661abd |
  |              metadataContentOf(uint256,uint256)              | 0x39fbc775 |
  |                      tokenUriResolver()                      | 0xe131fc0c |
  |             createFor(address,JBProjectMetadata)             | 0xf1158b5b |
  |           setMetadataOf(uint256,JBProjectMetadata)           | 0x4325ecef |
  |                 setTokenUriResolver(address)                 | 0x2407497e |
  |                      tokenURI(uint256)                       | 0xc87b56dd |
  |                  supportsInterface(bytes4)                   | 0x01ffc9a7 |
  |                     constructor(address)                     | 0xf8a6c595 |
  |             createFor(address,JBProjectMetadata)             | 0xf1158b5b |
  |           setMetadataOf(uint256,JBProjectMetadata)           | 0x4325ecef |
  |                 setTokenUriResolver(address)                 | 0x2407497e |
  |                       operatorStore()                        | 0xad007d63 |
  |                           count()                            | 0x06661abd |
  |              metadataContentOf(uint256,uint256)              | 0x39fbc775 |
  |                      tokenUriResolver()                      | 0xe131fc0c |
  +--------------------------------------------------------------+------------+

• Now, imagine, that somewhere in your contract you're calling tokenUriResolver() a lot of time. Maybe in loops, or large external calls and in order to make it happen we need to make 62 JUMPs each time. So, the optimization point here is to reduce amount of JUMPs by renaming them such that after getting the function's selector it will be placed closed to top.

Recommendations:

• Try to prioritize the frequency of declared functions and based on that figure out about placing them.

[G-04] Consider a = a + b instead of a += b<a name="G-04"></a>

Description:

• It has an impact on the deployment cost and the cost for distinct transaction.

All occurances:

• Contracts:

    file: contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol
    ...............................
    
    // Lines: [859-861]
    unchecked {
      _feeEligibleDistributionAmount += _leftoverDistributionAmount;
    }
  
    // Lines: [1038-1038]
    feeEligibleDistributionAmount += _payoutAmount;
  
    // Lines: [1103-1103]
    feeEligibleDistributionAmount += _payoutAmount;
  
    // Lines: [1145-1145]
    feeEligibleDistributionAmount += _payoutAmount;
  
    // Lines: [1401-1405]
      refundedFees += _feeAmount(
          _heldFees[_i].amount,
          _heldFees[_i].fee,
          _heldFees[_i].feeDiscount
      );
  
    // Lines: [1416-1416]
      unchecked {
        refundedFees += _feeAmount(leftoverAmount, _heldFees[_i].fee, _heldFees[_i].feeDiscount);
      }
      
      ```

[G-05] Consider marking onlyOwner functions as payable<a name="G-05"></a>

Description:

• A little optmization in comparison between payable and non-payable functions.

All occurances:

• Contracts:

    file: contracts/JBDirectory.sol
    ...............................
    
      // Lines: [333-336]
      function setIsAllowedToSetFirstController(address _address, bool _flag)
        external
        override
        onlyOwner
      {}
  
    file: contracts/JBPrices.sol
    ...............................
  
      // Lines: [106-113]
      function addFeedFor(
        uint256 _currency,
        uint256 _base,
        IJBPriceFeed _feed
      ) external override onlyOwner {}
  
    file: contracts/JBProjects.sol
    ...............................
  
      // Lines: [179-179]
      function setTokenUriResolver(IJBTokenUriResolver _newResolver) external override onlyOwner {}
  
    file: contracts/JBToken.sol
    ...............................
  
      // Lines: [106-110]
      function mint(
        uint256 _projectId,
        address _account,
        uint256 _amount
      ) external override onlyOwner {}
  
      // Lines: [127-131]
      function burn(
        uint256 _projectId,
        address _account,
        uint256 _amount
      ) external override onlyOwner {}
  
    file: contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol
    ...............................
  
      // Lines: [622-622]
      function setFee(uint256 _fee) external virtual override onlyOwner {}
  
      // Lines: [644-644]
      function setFeeGauge(IJBFeeGauge _feeGauge) external virtual override onlyOwner {}
  
      // Lines: [661-661]
      function setFeelessAddress(address _address, bool _flag) external virtual override onlyOwner {}
  
      ```

[G-06] Use constants instead of type(uint232).max<a name="G-06"></a>

Description:

• type(uint232).max or type(uint48).max, etc uses significant amount of gas on the deployment process and per each transaction as well.

Recommendations:

• Maybe, it's better to define some constants to store them? What about these type of operations in loops?

All occurances:

• Contracts:

    file: contracts/JBController.sol
    ...............................
    
      // Lines: [1018-1019]
      if (_constraints.distributionLimit > type(uint232).max) 
        revert INVALID_DISTRIBUTION_LIMIT();
  
      // Lines: [1021-1022]
      if (_constraints.distributionLimitCurrency > type(uint24).max)
        revert INVALID_DISTRIBUTION_LIMIT_CURRENCY();
  
      // Lines: [1025-1025]
      if (_constraints.overflowAllowance > type(uint232).max) revert INVALID_OVERFLOW_ALLOWANCE();
  
      // Lines: [1028-1029]
      if (_constraints.overflowAllowanceCurrency > type(uint24).max)
        revert INVALID_OVERFLOW_ALLOWANCE_CURRENCY();
  
    file: contracts/JBFundingCycleStore.sol
    ...............................
      
      // Lines: [306-307]
      if (_constraints.overflowAllowanceCurrency > type(uint24).max)
        revert INVALID_OVERFLOW_ALLOWANCE_CURRENCY();
  
      // Lines: [312-313]
      if (_data.weight > type(uint88).max) revert INVALID_WEIGHT(); 
  
    file: contracts/JBSplitsStore.sol
    ...............................
      
      // Lines: [234-234]
      if (_splits[_i].projectId > type(uint56).max) revert INVALID_PROJECT_ID();
  
      // Lines: [261-261]
      if (_splits[_i].lockedUntil > type(uint48).max) revert INVALID_LOCKED_UNTIL();
  

Kudos for the quality of the code! It's pretty easy to explore!