Juicebox V2 contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

MED - the function of the protocol could be impacted

1. Use stale price information resulting to wrong project's balance
2. In the case of zero price, functions using price information will revert.

Proof of Concept

// JBPrices::priceFor at line 69 calls _feed.currentPrice

 57   function priceFor(
 58     uint256 _currency,
 59     uint256 _base,
 60     uint256 _decimals
 61   ) external view override returns (uint256) {
// ...
 68     // If it exists, return the price.
 69:    if (_feed != IJBPriceFeed(address(0))) return _feed.currentPrice(_decimals);

// JBChainlinkV3PriceFeed::currentPrice calls latestRoundData and uses only the price

 42   function currentPrice(uint256 _decimals) external view override returns (uint256) {
 43     // Get the latest round information. Only need the price is needed.
 44:    (, int256 _price, , , ) = feed.latestRoundData();
 45
 46     // Get a reference to the number of decimals the feed uses.
 47     uint256 _feedDecimals = feed.decimals();
 48
 49     // Return the price, adjusted to the target decimals.
 50     return uint256(_price).adjustDecimals(_feedDecimals, _decimals);

The JBPrices contract fetches price information from chainlink and the information is used in JBSingleTokenPaymentTerminalStore, to determine how much token should be issued, etc.

There are two issues with the line 44 of JBChainlinkV3PriceFeed

1. The returned information might be stale There are not checks on roundID nor timeStamp, resulting in the possibility to use stale price information.

2. It does not checked whether the returned value is greater than zero The returned price is the type of int256, so for some reason a negative value is passed as the answer, it will give a wrong value on the line 50 without warning. Likewise, if the passed price is zero, it will cause revert in the JBSingleTokenPaymentTerminalStore 1, 2, 3, because the value is used as denominator.

// example usage of the price information
// line 661 JBSingleTokenPaymentTerminalStore

658       : PRBMath.mulDiv(
659         _amount,
660         10**_MAX_FIXED_POINT_FIDELITY, // Use _MAX_FIXED_POINT_FIDELITY to keep as much of the `_amount.value`'s fidelity as possible when converting.
661:        prices.priceFor(_currency, _balanceCurrency, _MAX_FIXED_POINT_FIDELITY)
662       );

Tools Used

none

Recommended Mitigation Steps

1. validate data feed

// example
    (uint80 roundID, int256 _price, , uint256 timestamp, uint80 answeredInRound) = feed.latestRoundData();
    require(_price > 0, "ChainLink: price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale price");
    require(timestamp > 0, "ChainLink: Round not complete");

2. use try and catch upon calling on the datafeed and have a backup oracle

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L211-L221

Vulnerability details

Impact

MED - the function of the protocol could be impacted

Proof of Concept

• proof of concept

The proof of concept demonstrates to discard one of duplicated locked splits. In the beginning it launches a project with two identical locked splits. As the owner of the project, it updates splits to only one of the two splits. Since all of original splits are locked both of them should still in the split after the update, but only one of them exists in the updated splits.

It happens because the check of the locked split is not suitable for duplicated cases.

// JBSplitsStore::_set

203    // Check to see if all locked splits are included.
204    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
205      // If not locked, continue.
206      if (block.timestamp >= _currentSplits[_i].lockedUntil) continue;
207
208      // Keep a reference to whether or not the locked split being iterated on is included.
209      bool _includesLocked = false;
210
211      for (uint256 _j = 0; _j < _splits.length; _j++) {
212        // Check for sameness.
213        if (
214          _splits[_j].percent == _currentSplits[_i].percent &&
215          _splits[_j].beneficiary == _currentSplits[_i].beneficiary &&
216          _splits[_j].allocator == _currentSplits[_i].allocator &&
217          _splits[_j].projectId == _currentSplits[_i].projectId &&
218          // Allow lock extention.
219          _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil
220        ) _includesLocked = true;
221      }
222
223      if (!_includesLocked) revert PREVIOUS_LOCKED_SPLITS_NOT_INCLUDED();
224    }

Tools Used

None

Recommended Mitigation Steps

Either prevent duplicates in the splits or track the matches while checking the locked splits.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L306-L312 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBFundingCycleStore.sol#L518-L522

Vulnerability details

Impact

MED - the function of the protocol could be impacted

By setting huge mustStartAtOrAfter, the owner can set start time in the past. It might open up possibility to bypass the ballot waiting time depending on the ballot's implementation.

Proof of Concept

• proof of concept

The proof of concept is almost the same as TestReconfigure::testReconfigureProject. In the original test, the owner of the project is reconfiguring funding cycle, but it is not in effect immediately because ballot is set. Only after 3 days the newly set funding cycle will be the current one. In the above proof of concept, only one parameter of the funding cycle is modified: mustStartAtOrAfter is set to type(uint56).max. As the result, the newly set funding cycle is considered as the current one without waiting for the ballot.

The cause of this is missing check on mustStartAtOrAfter upon setting here. If the given _mustStartAtOrAfter is huge, it will be passed eventually to the _initFor, _packAndStoreIntrinsicPropertiesOf. Then it will 'overflow' by shifting and set to the funding cycle, which essentially can be set to any value including the past. Also, it seems like the number will be also effected because the bigger digit will carry over.

// in JBFundingCycleStore::_packAndStoreIntrinsicPropertiesOf
// where the `_start` is derived from `_mustStartAtOrAfter`

./JBFundingCycleStore.sol-518-    // start in bits 144-199.
./JBFundingCycleStore.sol:519:    packed |= _start << 144;
./JBFundingCycleStore.sol-520-
./JBFundingCycleStore.sol-521-    // number in bits 200-255.
./JBFundingCycleStore.sol-522-    packed |= _number << 200;

Tools Used

foundry

Recommended Mitigation Steps

Add a check for the _mustStartAtOrAfter:

// example check for _mustSTartAtOrAfter
// in JBFundingCycleStore::configureFor

if (_mustStartAtOrAfter > type(uint56).max) revert INVALID_START();

Lines of code

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

Vulnerability details

Impact

MED - hypothetical attack path with stated assumptions

Attacker can compromise assets by abusing missing return value check, when a project is using ERC20 token which does not revert on failure.

Proof of Concept

Some ERC20 tokens do not revert on failure for transfer, transferFrom and approve but return false. It is good to add checks on the return value if the contract is meant to be used for general ERC20 token.

// JBERC20PaymentTerminal.sol
// The return values of the call transfer, transferFrom and approve are not checked

 81   function _transferFrom(
 82     address _from,
 83     address payable _to,
 84     uint256 _amount
 85   ) internal override {
 86     _from == address(this)
 87       ? IERC20(token).transfer(_to, _amount)
 88       : IERC20(token).transferFrom(_from, _to, _amount);
 89   }
//...
 98   function _beforeTransferTo(address _to, uint256 _amount) internal override {
 99     IERC20(token).approve(_to, _amount);
100   }

If the owner of project is using some ERC20 tokens which does not revert on failure but return false, users can abuse the lack of check. The user does not approve to send any funds to the terminal and the transferFrom fails, but since the returned value is not checked the user will still get project's share. It is also possible to add the existing terminal with such a token to another project and fake the balance and get the other project's fund out.

Tools Used

none

Recommended Mitigation Steps

Add checks on the return values. But keep in mind some tokens do not return a bool (although it is the standard to return a bool). So to cover these cases use safeTransfer to check the return value only when there is one.

Juicebox V2 QA Report

• There are overall missing zero address checks in constructors and setters. Some of these inputs are set to immutable variables, so one cannot change even if it was set to zero as mistake. Also some of zero addresses lead other functions to revert.

Low

L00: blocking recordPaymentFrom, recordRedemptionFor by setting mismatching information

• code
  • JBSingleTokenPaymentTerminalStore::recordPaymentFrom can be blocked by setting fundingCycle.useDataSourceForPay to true and fundingCycle.dataSource to zero address.
• code
  • JBSingleTokenPaymentTerminalStore::recordRedemptionFor can be blocked by setting fundingCycle.useDataSourceForRedeem to true and fundingCycle.dataSource to zero address.

It happens because the metadata for fundingCycle is not checked.

However, the project owner can achieve the same result with pausing redemption or setting malicious dataSource contract. Therefore, reporting this issue as Low severity.

L01: can set operator for projectId zero

• code

When setting controller for a project, it does not check whether the projectId is zero. The projectId starts from one, so projectId 0 should be forbidden. Somebody in the isAllowedToSetFirstController list can set controller of zero projectId and set other variables for non existing zeroth project using the controller.

// JBDirectory::setControllerOf
// in the below check, add projectId

223    if (projects.count() < _projectId) revert INVALID_PROJECT_ID_IN_DIRECTORY();

L02: JBPayoutRedemptionPaymentTerminal cannot handle fee-on-transfer tokens

• code

// JBPayoutRedemptionPaymentTerminal::addToBalanceOf

      // Transfer tokens to this terminal from the msg sender.
      _transferFrom(msg.sender, payable(address(this)), _amount);

The above code assumes the transferred balance will be added to the balance without any loss, therefore failing to consider fee-on-transfer tokens. It will result in recorded balance and real balance mismatch. As the result, some people will get more money than they should get (basically the fee amount). Eventually there will be missing funds, and some people will fail to withdraw. When terminals for fee-on-transfer tokens are shared it leads to a problem. Since projects can add terminals which other projects are using, some malicious project owner can mass up balance of other projects when they use fee-on-transfer tokens.

It is reported as Low severity because it was unclear whether the platform is suppose to support these tokens.