Juicebox V2 contest - 0xf15ers's results

Lines of code

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

Vulnerability details

Impact

The result from price feed needs further validation for stale and incorrect results.

Proof of Concept

 function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
    (, int256 _price, , , ) = feed.latestRoundData(); // here
    
    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();
    .......
    }

Tools Used

• Manual analysis

Recommended Mitigation Steps

• Consider checks for stale data

(uint80 roundId, int256 _price, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData();
 require(answeredInRound >= roundID, "Stale price");
 ........

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L348 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L256

Vulnerability details

Impact

Some ERC20 token returns false intead of reverting on fail.

Proof of Concept

./contracts/JBERC20PaymentTerminal.sol:88:      : IERC20(token).transferFrom(_from, _to, _amount);
./contracts/JBETHERC20SplitsPayer.sol:256:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20SplitsPayer.sol:348:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20ProjectPayer.sol:271:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20ProjectPayer.sol:315:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

./contracts/JBERC20PaymentTerminal.sol:87:      ? IERC20(token).transfer(_to, _amount)
./contracts/JBETHERC20SplitsPayer.sol:301:          IERC20(_token).transfer(
./contracts/JBETHERC20SplitsPayer.sol:384:          IERC20(_token).transfer(
./contracts/JBETHERC20SplitsPayer.sol:534:            IERC20(_token).transfer(  

Tools Used

• Manual analysis

Recommended Mitigation Steps

• Consider using OpenZeppelin's safeTransfer of safeERC20 library

1. Add address(0) check while setting new addresses

It is recommended to check for zero address to save from unintentional consequences.

• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L125-L126
• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBSingleTokenPaymentTerminal.sol#L124-L132

2. Unused import

IJBFundingCycleStore.sol is imported but not used insde the interface.

• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/interfaces/IJBFundingCycleBallot.sol#L6

3. Large multiples of ten should use scientific notation (e.g. 1e9) rather than decimal literals (e.g. 1000000000), for readability

• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/libraries/JBConstants.sol#L9-L14

1. Cache array length in loops instead of computing length in every iteration

While looping through array, the array length can be cached to save gas instead of computing length in each array iteration.

for eg.

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

in:

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

2. Prefix increment (++i) is cheaper than postfix increment (i++)

Using prefix increment is saves small amount of gas than postfix increment because it returns the updated value hence doesn't requires to store intermediate value. This can be more significant in loops where this operation is done multiple times.

for eg.

// before
for(uint256 i = 0; i < len; i++) {
  ...
}

// Replace with
for(uint256 i = 0; i < len; ++i) {
  ...
}

In:

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

3. !=0 is more gas efficient than >0 for uints

For uint comparision, != 0 can be used instead of > 0 in order to save gas In:

./contracts/JBFundingCycleStore.sol:149:    if (_standbyFundingCycleConfiguration > 0) {
./contracts/JBFundingCycleStore.sol:210:    if (_fundingCycleConfiguration > 0) {
./contracts/JBFundingCycleStore.sol:347:      _data.duration > 0 ||
./contracts/JBFundingCycleStore.sol:348:      _data.discountRate > 0
./contracts/JBFundingCycleStore.sol:364:    if (_metadata > 0) _metadataOf[_projectId][_configuration] = _metadata;
./contracts/JBFundingCycleStore.sol:469:      _weight = _weight > 0
./contracts/JBFundingCycleStore.sol:560:      _baseFundingCycle.duration > 0 &&
./contracts/JBFundingCycleStore.sol:589:      _fundingCycle.duration > 0 && block.timestamp >= _fundingCycle.start + _fundingCycle.duration
./contracts/JBFundingCycleStore.sol:601:      _baseFundingCycle.duration > 0 &&
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:346:      if (msg.value > 0) revert NO_MSG_VALUE_ALLOWED();
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:516:    if (balance > 0) {
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:552:      if (msg.value > 0) revert NO_MSG_VALUE_ALLOWED();
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:745:      if (_tokenCount > 0)
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:772:    if (reclaimAmount > 0) _transferFrom(address(this), _beneficiary, reclaimAmount);
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:884:      if (netLeftoverDistributionAmount > 0)
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:964:      if (netDistributedAmount > 0)
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:1022:      if (_payoutAmount > 0) {
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:1297:      if (_tokenCount > 0)
./contracts/JBProjects.sol:143:    if (bytes(_metadata.content).length > 0)
./contracts/JBSingleTokenPaymentTerminalStore.sol:475:        if (_currentOverflow > 0)
./contracts/JBSingleTokenPaymentTerminalStore.sol:518:    if (reclaimAmount > 0)
./contracts/JBTokenStore.sol:356:    if (_unclaimedTokensToBurn > 0) {
./contracts/JBTokenStore.sol:367:    if (_claimedTokensToBurn > 0) _token.burn(_projectId, _holder, _claimedTokensToBurn);
./contracts/system_tests/TestDistributeHeldFee.sol:154:    if (fee > 0 && payAmountInWei > 0) {
./contracts/JBController.sol:438:    if (_terminals.length > 0) directory.setTerminalsOf(projectId, _terminals);
./contracts/JBController.sol:481:    if (fundingCycleStore.latestConfigurationOf(_projectId) > 0)
./contracts/JBController.sol:498:    if (_terminals.length > 0) directory.setTerminalsOf(_projectId, _terminals);
./contracts/JBController.sol:875:    if (_leftoverTokenCount > 0) tokenStore.mintFor(_owner, _projectId, _leftoverTokenCount, false);
./contracts/JBController.sol:925:      if (_tokenCount > 0) {
./contracts/JBController.sol:1032:      if (_constraints.distributionLimit > 0)
./contracts/JBController.sol:1040:      if (_constraints.overflowAllowance > 0)
./contracts/JBSplitsStore.sol:259:      if (_splits[_i].lockedUntil > 0 || _splits[_i].allocator != IJBSplitAllocator(address(0))) {
./contracts/JBSplitsStore.sol:272:      } else if (_packedSplitParts2Of[_projectId][_domain][_group][_i] > 0)
./contracts/JBSplitsStore.sol:326:      if (_packedSplitPart2 > 0) {  

4. Use unchecked math for conditions which will never overflow

Several operations such as the loop counter increment will never overflow since it starts from zero and only increases. In such cases unchecked math can be used to save gas.

for eg.

  // Change
  for (uint256 i = 0; i < orders.length; i++) {

  }

  // To
  for (uint256 i = 0; i < orders.length; ) {

    unchecked {
      i++;
    }
  }

5. Functions doesn't uses specified named return

• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L136
  • The function queuedOf has a named return fundingCycle but all cases of return is explicit
  • The function currentOf has a named return fundingCycle but all cases of return is explicit
  • The function get has a named return fundingCycle but all cases of return is explicit

6. Constants can be changed to private from public

Having public constants also makes their getter functions. This will increase code size and hence deployment gas. They can be changed to private to save gas.

• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/libraries/JBConstants.sol
• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/libraries/JBOperations.sol
• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/libraries/JBSplitsGroups.sol
• https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/libraries/JBTokens.sol