Juicebox V2 contest - hyh's results

Lines of code

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

Vulnerability details

updatedAt field of Chainlink's latestRoundData() isn't checked, so even substantially outdated price will be used by the system.

Proof of Concept

Price is the only field that is read from Chainlink:

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

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

Recommended Mitigation Steps

Consider adding a logic to filter out outdated or wrong submissions, for example:

(, int256 priceInUsd, , int256 updatedAt, ) = baseAggregator.latestRoundData();

require(priceInUsd > 0 && updatedAt > 0, "Price invalid");

// some checks that updatedAt isn't too far in the past

https://etherscan.io/address/0x37bc7498f4ff12c19678ee8fe19d713b87f6a9e6#code#F2#L786

https://docs.chain.link/docs/feed-registry/#latestrounddata

Lines of code

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

Vulnerability details

A number of functions (full list below) will not revert on the transfer call failure and can be inaccessible for tokens not fully compliant with OZ's IERC20 (say, USDT). Unifying this in one issue for all the affected functions.

1. When a token do not revert, but instead return false this will go unnoticed in the current implementation with the corresponding downstream system's failures on programmatic usage.

2. ERC20 compliance is expected by using transfer, while it is not always the case and non-compliant token will not be retrievable by the functions.

(2) means that such tokens will be frozen on the contract balance, and, as the functions mentioned are parts of core system functionality this means system unavailability and fund freezing: say Bob the DAO manager moved funds to the system, but it cannot manage them, the functions are unavailable, the funds are frozen.

Setting impact to be high as that both can break up the functionality and produce yield losses as a part of token freezing.

Proof of Concept

The project use standard OZ implementation:

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

import '@openzeppelin/contracts/token/ERC20/IERC20.sol';

The transfer is done with unsafe function version despite the fact that arbitrary tokens are meant to be moved in the following functions:

pay:

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

      // Transfer tokens to this contract from the msg sender.
      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

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

          IERC20(_token).transfer(
            // If there's a beneficiary, send the funds directly to the beneficiary. Otherwise send to the msg.sender.
            _beneficiary != address(0) ? _beneficiary : msg.sender,
            _leftoverAmount
          );

addToBalanceOf:

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

      // Transfer tokens to this contract from the msg sender.
      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

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

          IERC20(_token).transfer(
            // If there's a default beneficiary, send the funds directly to the beneficiary. Otherwise send to the msg.sender.
            defaultBeneficiary != address(0) ? defaultBeneficiary : msg.sender,
            _leftoverAmount
          );

_payTo:

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

            // Or, transfer the ERC20.
          else {
            IERC20(_token).transfer(
              // Get a reference to the address receiving the tokens. If there's a beneficiary, send the funds directly to the beneficiary. Otherwise send to _defaultBeneficiary.
              _split.beneficiary != address(0) ? _split.beneficiary : _defaultBeneficiary,
              _splitAmount
            );
          }

pay:

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L270-L271

      // Transfer tokens to this contract from the msg sender.
      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

addToBalanceOf:

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L314-L315

      // Transfer tokens to this contract from the msg sender.
      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

_transferFrom:

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

    _from == address(this)
      ? IERC20(token).transfer(_to, _amount)
      : IERC20(token).transferFrom(_from, _to, _amount);

To sum up:

1. The success of transfer operation isn't checked, so for tokens that do not revert on a failure, returning false instead, this failure will be ignored and downstream systems that rely on the functionality may malfunction (i.e. wrong internal accounting => insolvency kind of total impact).

2. Also, some tokens to not adhere to IERC20 fully, not returning the state at all (USDT as an example), for them the transfer will be failing as not complying to the interface:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L34-L41

https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code

Recommended Mitigation Steps

Consider using safeTransfer for all ERC20 in/outbound transfers.

Lines of code

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

Vulnerability details

Currently the fee is understated as the implementation calculates (1 / (1 + 1 / fee_share)) instead of fee_share. It is not described in the documentation, common plain fee mechanics is declared, so it looks like it's unintentional.

Making it high as that's the base functionality that not working as expected with a cumulative negative impact on DAO fee earnings.

Proof of Concept

_feeAmount() calculates (1 / (1 + 1 / fee_share)):

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

    // The amount of tokens from the `_amount` to pay as a fee.
    return
      _amount - PRBMath.mulDiv(_amount, JBConstants.MAX_FEE, _discountedFee + JBConstants.MAX_FEE);
  }

It is _amount * (_discountedFee / (_discountedFee + JBConstants.MAX_FEE)). If you denote fee_share = _discountedFee / JBConstants.MAX_FEE, when _discountedFee > 0 the fraction can be divided by it and the returning value can be written as _amount * (1 / (1 + 1 / fee_share)).

For example, when _discountedFee = JBConstants.MAX_FEE it becomes _amount / 2, when _discountedFee = JBConstants.MAX_FEE / 2 it becomes _amount / 3 and so on.

Plain fee percentage, _amount * fee_share, is declared in the description:

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

    @param _fee The percentage of the fee, out of MAX_FEE. 

https://info.juicebox.money/dev/api/contracts/or-payment-terminals/or-abstract/jbpayoutredemptionpaymentterminal/properties/fee

Recommended Mitigation Steps

There looks to be nothing malicious in the formula, but plain fee mechanics will be expected by system users. I.e. when you set the fee to be 50% of 5% = max_fee, you expect 2.5%, not 1.(6)%.

Consider making the calculation fit the description and the common fee mechanics:

    // The amount of tokens from the `_amount` to pay as a fee.
    return PRBMath.mulDiv(_amount, _discountedFee, JBConstants.MAX_FEE);
  }

  /** 
    @notice 
    Returns the fee amount based on the provided amount for the specified project.
    @param *_amount The amount that the fee is based on*, as a fixed point number with the same amount of decimals as this terminal.