Juicebox V2 contest - durianSausage's results

Lines of code

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

Vulnerability details

Medium Risk Findings

M01: USE SAFETRANSFERFROM INSTEAD OF TRANSFERFROM

problem

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible.

prof

JBERC20PaymentTerminal.sol, L88, _transferFrom

IERC20(token).transferFrom(_from, _to, _amount);

we should use

IERC20(token).safeTransferFrom(_from, _to, _amount);

Low Risk and Non-Critical Issues

L01: usage storage variable many times should store in memory temp variable

problem

If we use storage variable without modifying its value, we should use SLOAD once and MLOAD others.

prof

---

JBController.sol, 983, _configure

    if (_metadata.reservedRate > JBConstants.MAX_RESERVED_RATE) revert INVALID_RESERVED_RATE();

    // Make sure the provided redemption rate is valid.
    if (_metadata.redemptionRate > JBConstants.MAX_REDEMPTION_RATE)
      revert INVALID_REDEMPTION_RATE();

    // Make sure the provided ballot redemption rate is valid.
    if (_metadata.ballotRedemptionRate > JBConstants.MAX_REDEMPTION_RATE)
      revert INVALID_BALLOT_REDEMPTION_RATE();

we should convert it into

    uint256 max_redemption_rate = JBConstants.MAX_RESERVED_RATE;
    if (_metadata.reservedRate > max_redemption_rate) revert INVALID_RESERVED_RATE();

    // Make sure the provided redemption rate is valid.
    if (_metadata.redemptionRate > max_redemption_rate)
      revert INVALID_REDEMPTION_RATE();

    // Make sure the provided ballot redemption rate is valid.
    if (_metadata.ballotRedemptionRate > max_redemption_rate)
      revert INVALID_BALLOT_REDEMPTION_RATE();

---

JBController.sol, 1069, _reservedTokenAmountFrom

    // If all tokens are reserved, return the full unprocessed amount.
    if (_reservedRate == JBConstants.MAX_RESERVED_RATE) return _unprocessedTokenBalanceOf;

    return
      PRBMath.mulDiv(
        _unprocessedTokenBalanceOf,
        JBConstants.MAX_RESERVED_RATE,
        JBConstants.MAX_RESERVED_RATE - _reservedRate
      ) - _unprocessedTokenBalanceOf;

we should convert it into

    uint256 max_reserved_rate = JBConstants.MAX_RESERVED_RATE;
    // If all tokens are reserved, return the full unprocessed amount.
    if (_reservedRate == max_reserved_rate) return _unprocessedTokenBalanceOf;

    return
      PRBMath.mulDiv(
        _unprocessedTokenBalanceOf,
        max_reserved_rate,
        max_reserved_rate - _reservedRate
      ) - _unprocessedTokenBalanceOf;

---

JBFundingCycleStore.sol, 704, _deriveWeightFrom

    if (_baseFundingCycle.duration == 0)
      return
        PRBMath.mulDiv(
          _baseFundingCycle.weight,
          JBConstants.MAX_DISCOUNT_RATE - _baseFundingCycle.discountRate,
          JBConstants.MAX_DISCOUNT_RATE
        );

we should convert it into

    uint256 max_discount_rate = JBConstants.MAX_DISCOUNT_RATE;
    if (_baseFundingCycle.duration == 0)
      return
        PRBMath.mulDiv(
          _baseFundingCycle.weight,
          max_discount_rate - _baseFundingCycle.discountRate,
          max_discount_rate
        );

---

JBFundingCycleStore.sol, 727, _deriveWeightFrom

      weight = PRBMath.mulDiv(
        weight,
        JBConstants.MAX_DISCOUNT_RATE - _baseFundingCycle.discountRate,
        JBConstants.MAX_DISCOUNT_RATE
      );

we should convert it into

      weight = PRBMath.mulDiv(
        weight,
        max_discount_rate - _baseFundingCycle.discountRate,
        max_discount_rate
      );

---

JBSingleTokenPaymentTerminalStore.sol, 580, recordDistributionFor

    distributedAmount = (_currency == _balanceCurrency)
      ? _amount
      : PRBMath.mulDiv(
        _amount,
        10**_MAX_FIXED_POINT_FIDELITY, // Use _MAX_FIXED_POINT_FIDELITY to keep as much of the `_amount.value`'s fidelity as possible when converting.
        prices.priceFor(_currency, _balanceCurrency, _MAX_FIXED_POINT_FIDELITY)
      );

we use convert it into

    uint256 max_fixed_point_fidelity = _MAX_FIXED_POINT_FIDELITY;
    distributedAmount = (_currency == _balanceCurrency)
      ? _amount
      : PRBMath.mulDiv(
        _amount,
        10**max_fixed_point_fidelity, // Use _MAX_FIXED_POINT_FIDELITY to keep as much of the `_amount.value`'s fidelity as possible when converting.
        prices.priceFor(_currency, _balanceCurrency, max_fixed_point_fidelity)
      );

---

JBSingleTokenPaymentTerminalStore.sol, 775, _reclaimableOverflowDuring

    if (_redemptionRate == JBConstants.MAX_REDEMPTION_RATE) return _base;

    return
      PRBMath.mulDiv(
        _base,
        _redemptionRate +
          PRBMath.mulDiv(
            _tokenCount,
            JBConstants.MAX_REDEMPTION_RATE - _redemptionRate,
            _totalSupply
          ),
        JBConstants.MAX_REDEMPTION_RATE
      );

we use convert it into

    uint256 max_redemption_rate = JBConstants.MAX_REDEMPTION_RATE;
    if (_redemptionRate == max_redemption_rate) return _base;

    return
      PRBMath.mulDiv(
        _base,
        _redemptionRate +
          PRBMath.mulDiv(
            _tokenCount,
            max_redemption_rate - _redemptionRate,
            _totalSupply
          ),
        max_redemption_rate
      );

---

JBSingleTokenPaymentTerminalStore.sol, 827, _overflowDuring

     _distributionLimitRemaining = PRBMath.mulDiv(
        _distributionLimitRemaining,
        10**_MAX_FIXED_POINT_FIDELITY, // Use _MAX_FIXED_POINT_FIDELITY to keep as much of the `_amount.value`'s fidelity as possible when converting.
        prices.priceFor(_distributionLimitCurrency, _balanceCurrency, _MAX_FIXED_POINT_FIDELITY)
      );

we use convert it into

     uint256 max_fixed_point_dielity = _MAX_FIXED_POINT_FIDELITY;
     _distributionLimitRemaining = PRBMath.mulDiv(
        _distributionLimitRemaining,
        10**max_fixed_point_dielity, // Use _MAX_FIXED_POINT_FIDELITY to keep as much of the `_amount.value`'s fidelity as possible when converting.
        prices.priceFor(_distributionLimitCurrency, _balanceCurrency, max_fixed_point_dielity)
      );

---

JBSingleTokenPaymentTerminalStore.sol, 866, _currentTotalOverflowOf

    uint256 _totalOverflow18Decimal = _currency == JBCurrencies.ETH
      ? _ethOverflow
      : PRBMath.mulDiv(_ethOverflow, 10**18, prices.priceFor(JBCurrencies.ETH, _currency, 18));

we use convert it into

   uint256 eth_address = JBCurrencies.ETH;
   uint256 _totalOverflow18Decimal = _currency == eth_address
      ? _ethOverflow
      : PRBMath.mulDiv(_ethOverflow, 10**18, prices.priceFor(eth_address, _currency, 18));

gas optimization

G01: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

'JBController.sol', 437, 'JBController.sol', 480, 'JBController.sol', 497, 'JBController.sol', 874, 'JBController.sol', 924, 'JBController.sol', 1031 'JBController.sol', 1039 'JBController.sol', 437 'JBController.sol', 497 'JBETHERC20ProjectPayer.sol', 267, 'JBETHERC20ProjectPayer.sol', 311, 'JBETHERC20SplitsPayer.sol', 252, 'JBETHERC20SplitsPayer.sol', 274, 'JBETHERC20SplitsPayer.sol', 344, 'JBETHERC20SplitsPayer.sol', 366, 'JBETHERC20SplitsPayer.sol', 476, 'JBFundingCycleStore.sol', 148, 'JBFundingCycleStore.sol', 209, 'JBFundingCycleStore.sol', 346, 'JBFundingCycleStore.sol', 347, 'JBFundingCycleStore.sol', 363, 'JBFundingCycleStore.sol', 468, 'JBFundingCycleStore.sol', 559, 'JBFundingCycleStore.sol', 588, 'JBFundingCycleStore.sol', 600, 'JBFundingCycleStore.sol', 588 'JBProjects.sol', 142 'JBProjects.sol', 142 'JBSingleTokenPaymentTerminalStore.sol', 474 'JBSingleTokenPaymentTerminalStore.sol', 517 'JBSplitsStore.sol', 258 'JBSplitsStore.sol', 325 'JBPayoutRedemptionPaymentTerminal.sol', 345 'JBPayoutRedemptionPaymentTerminal.sol', 515 'JBPayoutRedemptionPaymentTerminal.sol', 551 'JBPayoutRedemptionPaymentTerminal.sol', 744 'JBPayoutRedemptionPaymentTerminal.sol', 771 'JBPayoutRedemptionPaymentTerminal.sol', 883 'JBPayoutRedemptionPaymentTerminal.sol', 963 'JBPayoutRedemptionPaymentTerminal.sol', 1021 'JBPayoutRedemptionPaymentTerminal.sol', 1296

G02: ARRAY LENTH SHOULD USE MEMoRY VARIABLE LOAD IT

problem

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP (3 gas), and gets rid of the extra DUP needed to store the stack offset

prof

JBController.sol', 912, JBController.sol', 1013 JBDirectory.sol', 138 JBDirectory.sol', 166 'JBDirectory.sol', 274 'JBDirectory.sol', 275 'JBETHERC20SplitsPayer.sol', 465 'JBOperatorStore.sol', 84 'JBOperatorStore.sol', 134 'JBOperatorStore.sol', 164 'JBSingleTokenPaymentTerminalStore.sol', 861 'JBSplitsStore.sol', 203 'JBSplitsStore.sol', 210 'JBSplitsStore.sol', 228 'JBPayoutRedemptionPaymentTerminal.sol', 1007

G04: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

JBController.sol', 912, JBController.sol', 1013 JBDirectory.sol', 138, JBDirectory.sol', 166, JBDirectory.sol', 274, JBDirectory.sol', 275, 'JBETHERC20SplitsPayer.sol', 465 'JBFundingCycleStore.sol', 723 JBOperatorStore.sol', 84, JBOperatorStore.sol', 134, JBOperatorStore.sol', 164, JBSingleTokenPaymentTerminalStore.sol', 861 JBSplitsStore.sol', 203 JBSplitsStore.sol', 210 JBSplitsStore.sol', 228 JBSplitsStore.sol', 303

G05: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

JBPayoutRedemptionPaymentTerminal.sol, 859 JBPayoutRedemptionPaymentTerminal.sol, 1037 JBPayoutRedemptionPaymentTerminal.sol, 1102 JBPayoutRedemptionPaymentTerminal.sol, 1144 JBPayoutRedemptionPaymentTerminal.sol, 1400 JBPayoutRedemptionPaymentTerminal.sol, 1416