Juicebox V2 contest - horsefacts's results

Lines of code

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

Vulnerability details

JBChainlinkV3PriceFeed#currentPrice reads the price value from the underlying Chainlink price feed, but ignores the other values returned by latestRoundData, which include the round timestamps and round ID in which the returned price was computed. These values should be used to verify that the reported price is not stale.

JBChainlinkV3PriceFeed#currentPrice:

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
    (, int256 _price, , , ) = feed.latestRoundData();

    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();

    // Return the price, adjusted to the target decimals.
    return uint256(_price).adjustDecimals(_feedDecimals, _decimals);
  }

Impact: Prices calculated by project payment terminals may be inaccurate, allowing users to claim and redeem project tokens at the wrong exchange rate.

Recommendation: Validate the oracle response for freshness, round completion, and nonzero price:

• Freshness: If answeredInRound is less than the returned roundID, the price is stale.
• Completion: If the updatedAt timestamp of the round is zero, the round is incomplete.
• Nonzero price: Although very unlikely, it's technically possible for the feed to return a negative price. This would cause an underflow when converted on L#50.

  error STALE_PRICE();
  error INCOMPLETE_ROUND();
  error NEGATIVE_PRICE();

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
    (uint80 roundId, int256 _price, , uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData();
    if(answeredInRound < roundID) revert STALE_PRICE();
    if(updatedAt == 0) revert INCOMPLETE_ROUND();
    if(_price < 0) revert NEGATIVE_PRICE();

    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();

    // Return the price, adjusted to the target decimals.
    return uint256(_price).adjustDecimals(_feedDecimals, _decimals);
  }

Lines of code

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

Vulnerability details

JBERC20PaymentTerminal#_transferFrom calls IERC20#transfer and transferFrom directly. There are two issues with using this interface directly:

1. JBERC20PaymentTerminal#_transferFrom function does not check the return value of these calls. Tokens that return false rather than revert to indicate failed transfers may silently fail rather than reverting as expected.

2. Since the IERC20 interface requires a boolean return value, attempting to transfer ERC20s with missing return values will revert. This means Juicebox payment terminals cannot support a number of popular ERC20s, including USDT and BNB.

JBERC20PaymentTerminal#_transferFrom:

  function _transferFrom(
    address _from,
    address payable _to,
    uint256 _amount
  ) internal override {
    _from == address(this)
      ? IERC20(token).transfer(_to, _amount)
      : IERC20(token).transferFrom(_from, _to, _amount);
  }

Impact: Juicebox payment terminals may issue project tokens to users even though their incoming token transfer failed. Juicebox payment terminals cannot support USDT, BNB, and other popular (but nonstandard) ERC20s.

Recommendation: Use a safe transfer library like OpenZeppelin SafeERC20 to ensure consistent handling of ERC20 return values and abstract over inconsistent ERC20 implementations.

Additionally, since payment terminals are meant to support a variety of ERC20s, consider writing simulation tests that make token transfers using payment terminals for the most popular and most unusual ERC20s.

(Note also that the out of scope JBETHERC20ProjectPayer and JBETHERC20SplitsPayer contracts also call IERC20#transfer and transferFrom without a helper!)

See the following Forge test, which simulates an attempted USDT transfer. (Run this in fork mode using the --fork-url flag).

// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import './helpers/TestBaseWorkflow.sol';
import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';

address constant USDT_ADDRESS = address(0xdAC17F958D2ee523a2206206994597C13D831ec7);

contract TestWeirdERC20 is TestBaseWorkflow {
  using SafeERC20 for IERC20Metadata;

  JBController controller;
  JBProjectMetadata _projectMetadata;
  JBFundingCycleData _data;
  JBFundingCycleMetadata _metadata;
  JBGroupedSplits[] _groupedSplits;
  JBFundAccessConstraints[] _fundAccessConstraints;
  IJBPaymentTerminal[] _terminals;
  JBTokenStore _tokenStore;
  JBERC20PaymentTerminal _tetherTerminal;

  IERC20Metadata usdt = IERC20Metadata(USDT_ADDRESS);
  address _projectOwner;

  uint256 WEIGHT = 1000 * 10**18;

  function setUp() public override {
    super.setUp();

    _projectOwner = multisig();

    _tokenStore = jbTokenStore();

    controller = jbController();

    _projectMetadata = JBProjectMetadata({content: 'myIPFSHash', domain: 1});

    _data = JBFundingCycleData({
      duration: 14,
      weight: WEIGHT,
      discountRate: 450000000,
      ballot: IJBFundingCycleBallot(address(0))
    });

    _metadata = JBFundingCycleMetadata({
      global: JBGlobalFundingCycleMetadata({allowSetTerminals: false, allowSetController: false}),
      reservedRate: 5000, //50%
      redemptionRate: 5000, //50%
      ballotRedemptionRate: 0,
      pausePay: false,
      pauseDistributions: false,
      pauseRedeem: false,
      pauseBurn: false,
      allowMinting: false,
      allowChangeToken: false,
      allowTerminalMigration: false,
      allowControllerMigration: false,
      holdFees: false,
      useTotalOverflowForRedemptions: false,
      useDataSourceForPay: false,
      useDataSourceForRedeem: false,
      dataSource: address(0)
    });

    _tetherTerminal = new JBERC20PaymentTerminal(
      usdt,
      jbLibraries().ETH(), // currency
      jbLibraries().ETH(), // base weight currency
      1, // JBSplitsGroupe
      jbOperatorStore(),
      jbProjects(),
      jbDirectory(),
      jbSplitsStore(),
      jbPrices(),
      jbPaymentTerminalStore(),
      multisig()
    );
    evm.label(address(_tetherTerminal), 'TetherTerminal');

    _terminals.push(_tetherTerminal);
  }

  function testTetherPaymentsRevert() public {
    JBERC20PaymentTerminal terminal = _tetherTerminal;

    _fundAccessConstraints.push(
      JBFundAccessConstraints({
        terminal: terminal,
        token: address(USDT_ADDRESS),
        distributionLimit: 10 * 10**18,
        overflowAllowance: 5 * 10**18,
        distributionLimitCurrency: jbLibraries().ETH(),
        overflowAllowanceCurrency: jbLibraries().ETH()
      })
    );

    uint256 projectId = controller.launchProjectFor(
      _projectOwner,
      _projectMetadata,
      _data,
      _metadata,
      block.timestamp,
      _groupedSplits,
      _fundAccessConstraints,
      _terminals,
      ''
    );

    address caller = msg.sender;
    evm.label(caller, 'caller');
    deal(address(usdt), caller, 20 * 10**18);

    evm.prank(caller);
    usdt.safeApprove(address(terminal), 20 * 10**18);
    evm.prank(caller);
    terminal.pay(
      projectId,
      20 * 10**18,
      address(usdt),
      msg.sender,
      0,
      false,
      'Forge test',
      new bytes(0)
    );
  }
}

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L109-L121

Vulnerability details

The owner of JBPrices may call addFeedFor once and only once per currency/base pair:

JBPrices#addFeedFor

  function addFeedFor(
    uint256 _currency,
    uint256 _base,
    IJBPriceFeed _feed
  ) external override onlyOwner {
    // There can't already be a feed for the specified currency.
    if (feedFor[_currency][_base] != IJBPriceFeed(address(0))) revert PRICE_FEED_ALREADY_EXISTS();

    // Store the feed.
    feedFor[_currency][_base] = _feed;

    emit AddFeed(_currency, _base, _feed);
  }

Although the project docs indicate that this is by design, this seems risky: in the event of a malicious, misconfigured, or malfunctioning price feed, there is no mechanism to pause or remove it. It would be possible to migrate to new currency IDs, but this would require redeploying payment terminals and reconfiguring existing projects.

Recommendation: Consider adding a method to pause and update a price feed in case of emergency.

QA Report

I really enjoyed reading the Juicebox codebase. Thanks for publishing and maintaining great documentation. Your risks page is a clear description of the project's threat model and the risks you've already considered. I found it very useful during this review. Beyond the docs, I appreciate the Juicebox philosophy of building an open, extensible protocol.

You have a good suite of unit and system tests in place, but I recommend writing additional fork tests around components that will interact with external tokens, especially payment terminals. It's possible to write tests that call the "real" token code in a forked test environment that precisely simulates real world conditions, which will help ensure payment terminals correctly handle all the edge cases in popular ERC20 tokens.

Low

Avoid ERC20 approve

JBERC20PaymentTerminal.sol calls IERC20#approve directly:

  function _beforeTransferTo(address _to, uint256 _amount) internal override {
    IERC20(token).approve(_to, _amount);
  }

Since the IERC20 interface expects a boolean return value, this may revert for weird ERC20s that do not return a bool from approve, limiting compatibility with these tokens.

It's pretty wild that there is still no good solution to this problem, but the best option is probably still to use OZ SafeERC20#safeApprove. Note that it's important to zero the allowance first before setting approval to a nonzero value:

  function _beforeTransferTo(address _to, uint256 _amount) internal override {
    IERC20(token).safeApprove(_to, 0);
    IERC20(token).safeApprove(_to, _amount);
  }

Informational

Prefer two-step ownership transfers

OpenZeppelin Ownable uses single step ownership transfers. If the owner address ogf an Ownable contract is set to zero, transferred to an incorrect address, or accidentally renounced, ownership of these contracts may be permanently lost.

Consider introducing a mechanism for two-step ownership transfers.

Use address.code.length

JBFundingCycleStore#configureFor uses inline assembly to check whether the _ballot address is a contract with deployed code. In Solidity version 0.8.1 and later, address.code.length returns codesize using the extcodesize opcode.

JBFundingCycleStore#configureFor:

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

Suggestion:

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

Remove placeholder reference

There is a placeholder reference to the _account argument in JBToken#balanceOf intended to prevent a compiler warning:

function balanceOf(address _account, uint256 _projectId)
    external
    view
    override
    returns (uint256)
  {
    _account; // Prevents unused var compiler and natspec complaints.
    _projectId; // Prevents unused var compiler and natspec complaints.

    return super.balanceOf(_account);
  }

However, _account is used on L#58, so this reference can be removed.

Gas

Optimize loops in JBSplitsStore#_set

JBSplitsStore#set makes use of a couple common loop optimization tricks: caching array length and using unchecked preincrements:

  function set(
    uint256 _projectId,
    uint256 _domain,
    JBGroupedSplits[] calldata _groupedSplits
  )
    external
    override
    requirePermissionAllowingOverride(
      projects.ownerOf(_projectId),
      _projectId,
      JBOperations.SET_SPLITS,
      address(directory.controllerOf(_projectId)) == msg.sender
    )
  {
    // Push array length in stack
    uint256 _groupedSplitsLength = _groupedSplits.length;

    // Set each grouped splits.
    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
      // Get a reference to the grouped split being iterated on.
      JBGroupedSplits memory _groupedSplit = _groupedSplits[_i];

      // Set the splits for the group.
      _set(_projectId, _domain, _groupedSplit.group, _groupedSplit.splits);

      unchecked {
        ++_i;
      }
    }
  }

However, the internal function _set makes use of several loops (including a nested loop) that are not similarly optimized.

It's possible to make the same optimizations for the inner loops on L#204, L#211, and L#229. These sort of low level loop optimizations aren't always worth it, but since this function includes a nested loop, gas savings could add up as split sizes increase. I recommend profiling further and thinking through the likely length of splits in the real world to decide whether this is really worth it.

Gas before:

·----------------------------|---------------------------|-----------------|-----------------------------·
|    Solc version: 0.8.6     ·  Optimizer enabled: true  ·  Runs: 1000000  ·  Block limit: 30000000 gas  │
·····························|···························|·················|······························
|  Methods                                                                                               │
··················|··········|·············|·············|·················|···············|··············
|  Contract       ·  Method  ·  Min        ·  Max        ·  Avg            ·  # calls      ·  usd (avg)  │
··················|··········|·············|·············|·················|···············|··············
|  JBSplitsStore  ·  set     ·     100260  ·     277545  ·         194055  ·           16  ·          -  │
··················|··········|·············|·············|·················|···············|··············
|  Deployments               ·                                             ·  % of limit   ·             │
·····························|·············|·············|·················|···············|··············
|  JBOperations              ·          -  ·          -  ·         170485  ·        0.6 %  ·          -  │
·····························|·············|·············|·················|···············|··············
|  JBSplitsStore             ·    1157182  ·    1157194  ·        1157191  ·        3.9 %  ·          -  │
·----------------------------|-------------|-------------|-----------------|---------------|-------------·

Gas after:

·----------------------------|---------------------------|-----------------|-----------------------------·
|    Solc version: 0.8.6     ·  Optimizer enabled: true  ·  Runs: 1000000  ·  Block limit: 30000000 gas  │
·····························|···························|·················|······························
|  Methods                                                                                               │
··················|··········|·············|·············|·················|···············|··············
|  Contract       ·  Method  ·  Min        ·  Max        ·  Avg            ·  # calls      ·  usd (avg)  │
··················|··········|·············|·············|·················|···············|··············
|  JBSplitsStore  ·  set     ·      99630  ·     277259  ·         193687  ·           16  ·          -  │
··················|··········|·············|·············|·················|···············|··············
|  Deployments               ·                                             ·  % of limit   ·             │
·····························|·············|·············|·················|···············|··············
|  JBOperations              ·          -  ·          -  ·         170485  ·        0.6 %  ·          -  │
·····························|·············|·············|·················|···············|··············
|  JBSplitsStore             ·    1151986  ·    1151998  ·        1151995  ·        3.8 %  ·          -  │
·----------------------------|-------------|-------------|-----------------|---------------|-------------·

Code after. Note that it's important to handle the continue statement on L#206 correctly if you skip incrementing in the loop definition and manage increments in the loop body:

  function _set(
    uint256 _projectId,
    uint256 _domain,
    uint256 _group,
    JBSplit[] memory _splits
  ) internal {
    // Get a reference to the project's current splits.
    JBSplit[] memory _currentSplits = _getStructsFor(_projectId, _domain, _group);
    uint256 _currentLen = _currentSplits.length;
    uint256 _splitsLen = _splits.length;

    // Check to see if all locked splits are included.
    for (uint256 _i = 0; _i < _currentLen;) {
      // If not locked, continue.
      if (block.timestamp >= _currentSplits[_i].lockedUntil) {
        unchecked { ++_i; }
        continue;
      }

      // Keep a reference to whether or not the locked split being iterated on is included.
      bool _includesLocked = false;

      for (uint256 _j = 0; _j < _splits.length;) {
        // Check for sameness.
        if (
          _splits[_j].percent == _currentSplits[_i].percent &&
          _splits[_j].beneficiary == _currentSplits[_i].beneficiary &&
          _splits[_j].allocator == _currentSplits[_i].allocator &&
          _splits[_j].projectId == _currentSplits[_i].projectId &&
          // Allow lock extention.
          _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil
        ) _includesLocked = true;
        unchecked { ++_j; }
      }

      if (!_includesLocked) revert PREVIOUS_LOCKED_SPLITS_NOT_INCLUDED();
      unchecked { ++_i; }
    }

    // Add up all the percents to make sure they cumulative are under 100%.
    uint256 _percentTotal = 0;

    for (uint256 _i = 0; _i < _splitsLen;) {
      // The percent should be greater than 0.
      if (_splits[_i].percent == 0) revert INVALID_SPLIT_PERCENT();

      // ProjectId should be within a uint56
      if (_splits[_i].projectId > type(uint56).max) revert INVALID_PROJECT_ID();

      // Add to the total percents.
      _percentTotal = _percentTotal + _splits[_i].percent;

      // Validate the total does not exceed the expected value.
      if (_percentTotal > JBConstants.SPLITS_TOTAL_PERCENT) revert INVALID_TOTAL_PERCENT();

      uint256 _packedSplitParts1;

      // prefer claimed in bit 0.
      if (_splits[_i].preferClaimed) _packedSplitParts1 = 1;
      // prefer add to balance in bit 1.
      if (_splits[_i].preferAddToBalance) _packedSplitParts1 |= 1 << 1;
      // percent in bits 2-33.
      _packedSplitParts1 |= _splits[_i].percent << 2;
      // projectId in bits 32-89.
      _packedSplitParts1 |= _splits[_i].projectId << 34;
      // beneficiary in bits 90-249.
      _packedSplitParts1 |= uint256(uint160(address(_splits[_i].beneficiary))) << 90;

      // Store the first spit part.
      _packedSplitParts1Of[_projectId][_domain][_group][_i] = _packedSplitParts1;

      // If there's data to store in the second packed split part, pack and store.
      if (_splits[_i].lockedUntil > 0 || _splits[_i].allocator != IJBSplitAllocator(address(0))) {
        // Locked until should be within a uint48
        if (_splits[_i].lockedUntil > type(uint48).max) revert INVALID_LOCKED_UNTIL();

        // lockedUntil in bits 0-47.
        uint256 _packedSplitParts2 = uint48(_splits[_i].lockedUntil);
        // allocator in bits 48-207.
        _packedSplitParts2 |= uint256(uint160(address(_splits[_i].allocator))) << 48;

        // Store the second split part.
        _packedSplitParts2Of[_projectId][_domain][_group][_i] = _packedSplitParts2;

        // Otherwise if there's a value stored in the indexed position, delete it.
      } else if (_packedSplitParts2Of[_projectId][_domain][_group][_i] > 0)
        delete _packedSplitParts2Of[_projectId][_domain][_group][_i];

      emit SetSplit(_projectId, _domain, _group, _splits[_i], msg.sender);
      unchecked { ++_i; }
    }

    // Set the new length of the splits.
    _splitCountOf[_projectId][_domain][_group] = _splitsLen;
  }