Juicebox V2 contest - tintin's results

Lines of code

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

Vulnerability details

Impact

The JBChainlinkV3PriceFeed contract reads round data from a Chainlink AggregatorV3 price feed. It exposes the currentPrice() function which returns the reported price, normalized to its number of decimals.

However, neither round completeness or the quoted timestamp are checked to ensure that the reported price is not stale. Additionally, there is no check that the reported int256 price is not < 0 before it is cast to uint256. Thus if the reported price is negative then it will be cast into an extremely large number (uint256 max - x) which will be returned by currentPrice().

• The reported price can be negative depending on the data feed used. In this case, an arbitary feed is allowed.

Besides being vulnerable to these edge cases, if a malicious user is able to find a data feed which can report negative values (perhaps not ERC20, but lets say currencies in JB can be off-chain / external assets), they can manipulate it to temporarily (or permanently) force the priceFor() their mapped currency in JB to be valued at almost INT_MAX.

Proof of Concept

JBChainlinkV3PriceFeed.sol

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

Note that the other values returned by the call to latestRoundData() are omitted.

JBSingleTokenPaymentTerminalStore.sol:L385

    // Get a reference to the number of decimals in the amount. (prevents stack too deep).
    uint256 _decimals = _amount.decimals;

    // If the terminal should base its weight on a different currency from the terminal's currency, determine the factor.
    // The weight is always a fixed point mumber with 18 decimals. To ensure this, the ratio should use the same number of decimals as the `_amount`.
    uint256 _weightRatio = _amount.currency == _baseWeightCurrency
      ? 10**_decimals
      : prices.priceFor(_amount.currency, _baseWeightCurrency, _decimals);

    // Find the number of tokens to mint, as a fixed point number with as many decimals as `weight` has.
    tokenCount = PRBMath.mulDiv(_amount.value, _weight, _weightRatio);

Example usage of the .priceFor() function which is not explicitly permissioned (contract interface can be mimicked),

Tools Used

Manual review

Recommended Mitigation Steps

Add additional validation and guard against negative numbers:

  ...
   (uint80 roundID, int256 _price, , uint256 updatedAt, uint80 answeredInRound) = feed.latestRoundData();
  require(answeredInRound >= roundID, "ChainLink: Stale price");
  require(updatedAt != 0, "ChainLink: Round not complete");
  ...
  /// @audit prevent casting negative int to uint 
  if(_price < 0) {
      return 0;
  }