Juicebox V2 contest - Chom's results

Lines of code

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

Vulnerability details

Impact

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

On JBChainlinkV3PriceFeed.sol, we are using latestRoundData, but there is no check if the return value indicates stale data.

  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);
  }

This may be used in JBPrices.sol in priceFor function

  function priceFor(
    uint256 _currency,
    uint256 _base,
    uint256 _decimals
  ) external view override returns (uint256) {
    // If the currency is the base, return 1 since they are priced the same. Include the desired number of decimals.
    if (_currency == _base) return 10**_decimals;

    // Get a reference to the feed.
    IJBPriceFeed _feed = feedFor[_currency][_base];

    // If it exists, return the price.
    if (_feed != IJBPriceFeed(address(0))) return _feed.currentPrice(_decimals);

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

    // If it exists, return the inverse price.
    if (_feed != IJBPriceFeed(address(0)))
      return PRBMath.mulDiv(10**_decimals, 10**_decimals, _feed.currentPrice(_decimals));

    // No price feed available, revert.
    revert PRICE_FEED_NOT_FOUND();
  }

Tools Used

Compare with previous audit: https://code4rena.com/reports/2022-04-backd/#m-05-chainlinks-latestrounddata-might-return-stale-or-incorrect-results

Recommended Mitigation Steps

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information.
    (uint80 roundID, int256 _price, , uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData();
    require(answeredInRound >= roundID, "Stale price");
    require(answer > 0," Error.NEGATIVE_PRICE");
    require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_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 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L98-L100

Vulnerability details

Impact

JBERC20PaymentTerminal may not working with USDT due to following reasons

1. USDT doesn't have return value to indicate whether the transfer is success or not
2. USDT doesn't allow setting new allowance if allowance is not zero

Proof of Concept

1. USDT doesn't have return value to indicate whether the transfer is success or not

  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);
  }

2. USDT doesn't allow setting new allowance if allowance is not zero

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

If some line of code leave some dust USDT fund (Allowance not become zero after transfer). Consequence transfers will be reverted as it cannot be approved due to not having zero allowance.

Tools Used

Manual

Recommended Mitigation Steps

Use SafeERC20 library

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

using SafeERC20 for IERC20;

...

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

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

Wrong "message sender" comment should be "owner"

https://github.com/jbx-protocol/juice-contracts-v2/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L421-L422

    // Mint the project into the wallet of the message sender.
    projectId = projects.createFor(_owner, _projectMetadata);

Should be

    // Mint the project into the wallet of the owner.
    projectId = projects.createFor(_owner, _projectMetadata);

Caching the length in for loops and increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Caching the length in for loops:

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

for loop postcondition can be made unchecked Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

https://github.com/jbx-protocol/juice-contracts-v2/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L1014

for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

Can be optimized to

uint256 fundAccessConstraintsLength = _fundAccessConstraints.length;
for (uint256 _i; _i < _fundAccessConstraintsLength; ) {
...
unchecked { _i++; }
}

https://github.com/jbx-protocol/juice-contracts-v2/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L913

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

Can be optimized to

uint256 splitsLength = _splits.length;
for (uint256 _i = 0; _i < _splitsLength; ) {
...
unchecked { _i++; }
}