Juicebox V2 contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

A malicious project owner or an attacker can front-run the JBTokenStore.changeFor function and "steal" the _token for their own project. This token can then not be used for any other project as long as it's assigned to a project (due to projectOf[_token] != 0).

Best case, the "attacked" project owner deploys the custom token again.

Worst case, this already deployed and now "stolen"/"locked" custom token is already in use, has funds locked in the contract (for whatever reason) and transferred contract ownership already to the JBTokenStore contract. The token is now locked and unusable within Juicebox.

Proof of Concept

JBTokenStore.sol#L246

if (projectOf[_token] != 0) revert TOKEN_ALREADY_IN_USE();

Tools Used

Manual review

Recommended mitigation steps

Consider adding an (optional to use) allow-list functionality to the IJBToken interface (for instance, function setProjectId(uint256 projectId)) which the owner of the IJBToken can use to set an allowed projectId.

Then within the JBTokenStore.changeFor function, check this previously set projectId if it matches the function parameter _projectId.

Lines of code

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

Vulnerability details

Impact

Some tokens, like USDT (see requirement line 199), require first reducing the address allowance to 0 by calling approve(_spender, 0) and then approve the actual allowance.

When using one of these unsupported tokens, all transactions revert and the protocol cannot be used.

Proof of Concept

JBERC20PaymentTerminal._beforeTransferTo

function _beforeTransferTo(address _to, uint256 _amount) internal override {
  IERC20(token).approve(_to, _amount); // @audit-info will fail for common ERC-20 tokens like `USDT`
}

Tools Used

Manual review

Recommended mitigation steps

Approve with a zero amount first before setting the actual amount:

IERC20(token).approve(_to, 0); // @audit-info add this line to reduce allowance to 0 first
IERC20(token).approve(_to, _amount);

Lines of code

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

Vulnerability details

Impact

The JBPayoutRedemptionPaymentTerminal._feeAmount function is used to calculate the fee based on a given _amount, a fee rate _fee and an optional discount _feeDiscount.

However, the current implementation calculates the fee in a way that leads to inaccuracy and to fewer fees being paid than anticipated by the protocol.

Proof of Concept

JBPayoutRedemptionPaymentTerminal._feeAmount

function _feeAmount(
    uint256 _amount,
    uint256 _fee,
    uint256 _feeDiscount
  ) internal pure returns (uint256) {
    // Calculate the discounted fee.
    uint256 _discountedFee = _fee -
      PRBMath.mulDiv(_fee, _feeDiscount, JBConstants.MAX_FEE_DISCOUNT);

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

Example:

Given the following (don't mind the floating point arithmetic, this is only for simplicity. The issues still applies with integer arithmetic and higher decimal precision):

• amount - 1000
• fee - 5 (5%)
• feeDiscount - 10 (10%)
• MAX_FEE_DISCOUNT - 100
• MAX_FEE - 100

$discountedFee = fee - {{fee * feeDiscount} \over MAX_FEE_DISCOUNT}$
$discountedFee = 5 - {{5 * 10} \over 100}$
$discountedFee = 4.5$

Calculating the fee amount based on the discounted fee of $4.5$:

$fee_{Amount} = amount - {{amount * MAX_FEE} \over {discountedFee + MAX_FEE}}$
$fee_{Amount} = 1000 - {{1000 * 100} \over {4.5 + 100}}$
$fee_{Amount} = 1000 - 956.93779904$
$fee_{Amount} = 43.06220096$

The calculated and wrong fee amount is ~43, instead, it should be 45. The issue comes from dividing by _discountedFee + JBConstants.MAX_FEE.

Now the correct way:

I omitted the discountedFee calculation as this formula is correct.

$fee_{Amount} = {{amount * discountedFee} \over {MAX_FEE}}$
$fee_{Amount} = {{1000 * 4.5} \over {100}}$
$fee_{Amount} = 45$

Tools Used

Manual review

Recommended mitigation steps

Fix the discounted fee calculation by adjusting the formula to:

$fee_{Amount} = amount \ast {fee - fee \ast {discount \over MAX_{FEE_DISCOUNT}} \over MAX_{FEE}}$

In Solidity:

function _feeAmount(
    uint256 _amount,
    uint256 _fee,
    uint256 _feeDiscount
) internal pure returns (uint256) {
    // Calculate the discounted fee.
    uint256 _discountedFee = _fee -
      PRBMath.mulDiv(_fee, _feeDiscount, JBConstants.MAX_FEE_DISCOUNT);

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

Lines of code

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

Vulnerability details

Impact

The JBPrices contract address is stored as immutable in various other contracts (for instance JBPayoutRedemptionPaymentTerminal).

As the price feeds are custom implemented "adapter" contracts (e.g for Chainlink), which may need to be updated at some point (bugs?), those price feed adapter implementations can not be changed and the stored JBPrices contract address in the various contracts can also not be updated.

Hence, if there are any issues in the price feed contract implementations, it is very difficult (and expensive) to update to new contracts. It will require a complete re-deployment of the JBPrices contract as well as re-deployment of the surface contracts (e.g. JBPayoutRedemptionPaymentTerminal). Current Juicebox projects would need to be updated to use the newly deployed terminal contracts.

Proof of Concept

JBPrices.addFeedFor

if (feedFor[_currency][_base] != IJBPriceFeed(address(0))) revert PRICE_FEED_ALREADY_EXISTS();

Tools Used

Manual review

Recommended mitigation steps

Re-consider the decision for immutable price feeds and add the possibility to update existing price feeds.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L1086-L1090

Vulnerability details

Impact

The JBController.distributeReservedTokensOf function is used to distribute all outstanding reserved tokens for a project. Internally, the JBController._distributeReservedTokensOf function calculates the distributable amount of tokens tokenCount with the function JBController._reservedTokenAmountFrom.

However, the current implementation of JBController._reservedTokenAmountFrom calculates the amount of reserved tokens currently tracked in a way that leads to inaccuracy and to more tokens distributed than anticipated.

Impact: More tokens than publicly defined via the funding cycle reservedRate are distributed (minted) to the splits and the owner increasing the total supply and therefore reducing the amount of terminal assets redeemable by a user. The increased supply takes effect in JBSingleTokenPaymentTerminStore.recordRedemptionFor on L784. The higher the token supply, the less terminal assets redeemable.

Proof of Concept

JBController._reservedTokenAmountFrom

function _reservedTokenAmountFrom(
    int256 _processedTokenTracker,
    uint256 _reservedRate,
    uint256 _totalEligibleTokens
  ) internal pure returns (uint256) {
    // Get a reference to the amount of tokens that are unprocessed.
    uint256 _unprocessedTokenBalanceOf = _processedTokenTracker >= 0
      ? _totalEligibleTokens - uint256(_processedTokenTracker)
      : _totalEligibleTokens + uint256(-_processedTokenTracker);

    // If there are no unprocessed tokens, return.
    if (_unprocessedTokenBalanceOf == 0) return 0;

    // If all tokens are reserved, return the full unprocessed amount.
    if (_reservedRate == JBConstants.MAX_RESERVED_RATE) return _unprocessedTokenBalanceOf;

    return
      PRBMath.mulDiv(
        _unprocessedTokenBalanceOf,
        JBConstants.MAX_RESERVED_RATE,
        JBConstants.MAX_RESERVED_RATE - _reservedRate
      ) - _unprocessedTokenBalanceOf;
  }

Example:

Given the following (don't mind the floating point arithmetic, this is only for simplicity. The issues still applies with integer arithmetic and higher decimal precision):

• processedTokenTracker - -1000
• reservedRate - 10 (10%)
• totalEligibleTokens - 0
• MAX_RESERVED_RATE - 100

$unprocessedTokenBalanceOf = 0 + (--1000)$
$unprocessedTokenBalanceOf = 1000$

$reservedTokenAmount = {{unprocessedTokenBalanceOf * MAX_RESERVED_RATE} \over {MAX_RESERVED_RATE - reservedRate}} - unprocessedTokenBalanceOf$
$reservedTokenAmount = {{1000 * 100} \over {100 - 10}} - 1000$
$reservedTokenAmount = 1111.111 - 1000$
$reservedTokenAmount = 111,111$

The calculated and wrong amount is ~111, instead it should be 100 (10% of 1000). The issue comes from dividing by JBConstants.MAX_RESERVED_RATE - _reservedRate.

Now the correct way:

$reservedTokenAmount = {{unprocessedTokenBalanceOf * reservedRate} \over MAX_RESERVED_RATE}$
$reservedTokenAmount = {{1000 * 10} \over 100}$
$reservedTokenAmount = 100$

Tools Used

Manual review

Recommended mitigation steps

Fix the outstanding reserve token calculation by implementing the calculation as following:

function _reservedTokenAmountFrom(
    int256 _processedTokenTracker,
    uint256 _reservedRate,
    uint256 _totalEligibleTokens
) internal pure returns (uint256) {
  // Get a reference to the amount of tokens that are unprocessed.
  uint256 _unprocessedTokenBalanceOf = _processedTokenTracker >= 0
    ? _totalEligibleTokens - uint256(_processedTokenTracker)
    : _totalEligibleTokens + uint256(-_processedTokenTracker);

  // If there are no unprocessed tokens, return.
  if (_unprocessedTokenBalanceOf == 0) return 0;

  return
    PRBMath.mulDiv(
      _unprocessedTokenBalanceOf,
      _reservedRate,
      JBConstants.MAX_RESERVED_RATE
    );
}

Lines of code

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

Vulnerability details

Impact

The check if the newly provided project splits contain the currently locked splits does not check the JBSplit struct properties preferClaimed and preferAddToBalance.

According to the docs in JBSplit.sol, "...if the split should be unchangeable until the specified time, with the exception of extending the locked period.", locked sets are unchangeable.

However, locked sets with either preferClaimed or preferAddToBalance set to true can have their bool values overwritten by supplying the same split just with different bool values.

Proof of Concept

JBSplitsStore.sol#L213-L220

// Check for sameness.
if (
    _splits[_j].percent == _currentSplits[_i].percent &&
    _splits[_j].beneficiary == _currentSplits[_i].beneficiary &&
    _splits[_j].allocator == _currentSplits[_i].allocator &&
    _splits[_j].projectId == _currentSplits[_i].projectId &&
    // Allow lock extention.
    _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil
) _includesLocked = true;

The check for sameness does not check the equality of the struct properties preferClaimed and preferAddToBalance.

Tools Used

Manual review

Recommended mitigation steps

Add two additional sameness checks for preferClaimed and preferAddToBalance:

// Check for sameness.
if (
    _splits[_j].percent == _currentSplits[_i].percent &&
    _splits[_j].beneficiary == _currentSplits[_i].beneficiary &&
    _splits[_j].allocator == _currentSplits[_i].allocator &&
    _splits[_j].projectId == _currentSplits[_i].projectId &&
    _splits[_j].preferClaimed == _currentSplits[_i].preferClaimed && // @audit-info add check for sameness for property `preferClaimed`
    _splits[_j].preferAddToBalance == _currentSplits[_i].preferAddToBalance && // @audit-info add check for sameness for property `preferAddToBalance`
    // Allow lock extention.
    _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil
) _includesLocked = true;

QA Report

Table of Contents

• Non-Critical Findings
  • [NC-01] Replace assembly extcodesize checks
• Low Risk
  • [L-01] Comments suggesting wrong bit positions
  • [L-02] name and symbol are not enforced for custom project tokens
  • [L-03] memo is not passed on
  • [L-04] Potential division by 0

Non-Critical Findings

[NC-01] Replace assembly extcodesize checks

Description

Checking the code size of an account via the Yul extcodesize(...) expression can be replaced with <address>.code.length.

Findings

JBFundingCycleStore.sol#L319-L321

assembly {
    _size := extcodesize(_ballot)
}

Recommended mitigation steps

Change the code to the following:

if (_ballot.code.length == 0) revert INVALID_BALLOT();

Low Risk

[L-01] Comments suggesting wrong bit positions

Description

Split data is packed into 256 bits. Comments next to the bitwise operations explain the used bits. However, a few comments are off by a few bits.

Findings

JBSplitsStore.sol#L250

// projectId in bits 32-89. // @audit-info Should be `projectId in bits 34-89`
_packedSplitParts1 |= _splits[_i].projectId << 34;

JBSplitsStore.sol#L317

// projectId in bits 32-89. // @audit-info Should be `projectId in bits 34-89`
_split.projectId = uint256(uint56(_packedSplitPart1 >> 34));

Recommended mitigation steps

Fix the comments to mention the correct bit positions.

[L-02] name and symbol are not enforced for custom project tokens

Description

Contrary to JBTokenStore.issueFor, custom project tokens are not checked if they have a name and symbol property.

Findings

JBTokenStore.issueFor

name and symbol are required.

// There must be a name.
if (bytes(_name).length == 0) revert EMPTY_NAME();

// There must be a symbol.
if (bytes(_symbol).length == 0) revert EMPTY_SYMBOL();

JBTokenStore.changeFor

name and symbol are not checked for existence.

Recommended mitigation steps

Consider adding checks for name and symbol to the JBTokenStore.issueFor function.

[L-03] memo is not passed on

Findings

JBPayoutRedemptionPaymentTerminal.sol#L750

IJBController(directory.controllerOf(_projectId)).burnTokensOf(
  _holder,
  _projectId,
  _tokenCount,
  '', // @audit-info `_memo` is not passed along
  false
);

JBPayoutRedemptionPaymentTerminal.sol#L1303

beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(
  _projectId,
  _tokenCount,
  _beneficiary,
  '', // @audit-info `_memo` is not passed along
  _preferClaimedTokens,
  true
);

Recommended mitigation steps

Consider passing on the memo parameter.

[L-04] Potential division by 0

Description

In rare cases, the price of the currency can potentially be 0, which would result in a division by zero error.

Findings

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

return PRBMath.mulDiv(10**_decimals, 10**_decimals, _feed.currentPrice(_decimals));

Recommended mitigation steps

Consider checking the result of _feed.currentPrice(_decimals) and conditionally execute the mulDiv operation.