Juicebox V2 contest - pashov's results

Lines of code

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

Vulnerability details

Proof of Concept

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

When calling latestRoundData the code does not validate if the data is stale or plain wrong. In most occurrences of this problem there are missing stale data checks, but here the price is not checked if it is ≤=0 as well.

Impact

Having a wrong price can mess with protocol’s accounting and result in loss of funds for users or the protocol. Especially if oracle malfunctions and returns price that is negative, with current implementation this can become a big problem in the protocol.

Recommendation

Change the latestRoundData logic to the following (you can use custom errors instead require statements as well):

(roundId, rawPrice,, updatedAt, answeredInRound) = feed.latestRoundData();
require(rawPrice > 0, "Chainlink price <= 0");
require(updateTime != 0, "Incomplete round");
require(answeredInRound >= roundId, "Stale price");

Lines of code

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

Vulnerability details

Proof of Concept

// Get a reference to the project's held fees.
    JBFee[] memory _heldFees = _heldFeesOf[_projectId];

    // Delete the held fees.
    delete _heldFeesOf[_projectId];

    // Push array length in stack
    uint256 _heldFeeLength = _heldFees.length;

    // Process each fee.
    for (uint256 _i = 0; _i < _heldFeeLength; ) {
      // Get the fee amount.
      uint256 _amount = _feeAmount(
        _heldFees[_i].amount,
        _heldFees[_i].fee,
        _heldFees[_i].feeDiscount
      );

The code here loops over the array of _heldFeesOf[_projectId]. This array is getting values pushed into it without checking if the array has gotten too big https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1199

Impact

If the processFees function reverts, then all fees can be stuck in the contract, resulting in potentially large amount of funds getting stuck in it.

Recommendation

Limit _takeFeeFrom calls to revert if a large amount of fees still haven’t been processed. For example you can have something like

require(*heldFeesOf[*projectId].length < 10, "Process fees first before taking another fee" . This way you limit the number of elements in the array so it doesn’t get so big that the transaction will revert due to out of gas or hitting block gas limit errors.

Missing non-zero address checks for constructor arguments

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

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L369 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBTokenStore.sol#L160

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

Proof of Concept: the code in scope is missing non-zero address checks. Actually all constructors in the repository are missing non-zero address checks (linked only 4 of them)

Impact: mistakenly setting a variable to the zero address can lead to unexpected behaviour, loss of funds, a need to redeploy contracts or send a new tx

Recommended Mitigation Steps: add non-zero address checks for all ‘address’ casted type parameters in all constructors in repository.