Juicebox contest - Jeiwan's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L563-L566

Vulnerability details

Impact

The amounts redeemed in overflow redemption can be calculated incorrectly due to incorrect accounting of the outstanding number of reserved tokens.

Proof of Concept

Project contributors are allowed to redeem their NFT tokens for a portion of the overflow (excessive funded amounts). The amount a contributor receives is calculated as overflow * (user's redemption rate / total redemption weight), where user's redemption weight is the total contribution floor of all their NFTs and total redemption weight is the total contribution floor of all minted NFTs. Since the total redemption weight is the sum of individual contributor redemption weights, the amount they can redeem is proportional to their contribution.

However, the total redemption weight calculation incorrectly accounts outstanding reserved tokens (JBTiered721DelegateStore.sol#L563-L566):

// Add the tier's contribution floor multiplied by the quantity minted.
weight +=
  (_storedTier.contributionFloor *
    (_storedTier.initialQuantity - _storedTier.remainingQuantity)) +
  _numberOfReservedTokensOutstandingFor(_nft, _i, _storedTier);

Specifically, the number of reserved tokens is added to the weight of minted tokens. This disrupts the redemption amount calculation formula since the total redemption weight is in fact not the sum of individual contributor redemption weights.

Tools Used

Manual review

Recommended Mitigation Steps

Two options can be seen:

1. if the outstanding number of reserved tokens is considered minted (which seems to be so, judging by this logic) then it needs to be added to the quantity, i.e.:

   --- a/contracts/JBTiered721DelegateStore.sol
   +++ b/contracts/JBTiered721DelegateStore.sol
   @@ -562,8 +562,7 @@ contract JBTiered721DelegateStore is IJBTiered721DelegateStore {
         // Add the tier's contribution floor multiplied by the quantity minted.
         weight +=
           (_storedTier.contributionFloor *
   -          (_storedTier.initialQuantity - _storedTier.remainingQuantity)) +
   -        _numberOfReservedTokensOutstandingFor(_nft, _i, _storedTier);
   +          (_storedTier.initialQuantity - _storedTier.remainingQuantity +
   +           _numberOfReservedTokensOutstandingFor(_nft, _i, _storedTier)));
   
         unchecked {
           ++_i;

2. if it's not considered minted, then it shouldn't be counted at all.

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L534

Vulnerability details

Impact

A contributor won't get an NFT they're eligible for if the payment is made through a payment terminal that's supported by the project but not by the NFT delegate.

Proof of Concept

A Juicebox project can use multiple payment terminals to receive contributions (JBController.sol#L441-L442). Payment terminals are single token payment terminals (JBPayoutRedemptionPaymentTerminal.sol#L310) that support only one currency (JBSingleTokenPaymentTerminal.sol#L124-L132). Since projects can have multiple terminals, they can receive payments in multiple currencies.

However, the NFT delegate supports only one currency (JBTiered721Delegate.sol#L225):

function initialize(
  uint256 _projectId,
  IJBDirectory _directory,
  string memory _name,
  string memory _symbol,
  IJBFundingCycleStore _fundingCycleStore,
  string memory _baseUri,
  IJBTokenUriResolver _tokenUriResolver,
  string memory _contractUri,
  JB721PricingParams memory _pricing,
  IJBTiered721DelegateStore _store,
  JBTiered721Flags memory _flags
) public override {
  // Make the original un-initializable.
  require(address(this) != codeOrigin);
  // Stop re-initialization.
  require(address(store) == address(0));

  // Initialize the sub class.
  JB721Delegate._initialize(_projectId, _directory, _name, _symbol);

  fundingCycleStore = _fundingCycleStore;
  store = _store;
  pricingCurrency = _pricing.currency; // @audit only one currency is supported
  pricingDecimals = _pricing.decimals;
  prices = _pricing.prices;

  ...
}

When a payment is made in a currency that's supported by the project (via one of its terminals) but not by the NFT delegate, there's an attempt to convert the currency to a supported one (JBTiered721Delegate.sol#L527-L534):

if (_data.amount.currency == pricingCurrency) _value = _data.amount.value;
else if (prices != IJBPrices(address(0)))
  _value = PRBMath.mulDiv(
    _data.amount.value,
    10**pricingDecimals,
    prices.priceFor(_data.amount.currency, pricingCurrency, _data.amount.decimals)
  );
else return;

However, since prices is optional (it can be set to the zero address, as seen from the snippet), the conversion step can be skipped. When this happens, the contributor gets no NFT due to the early return even though the amount of their contribution might still be eligible for a tiered NFT.

Tools Used

Manual review

Recommended Mitigation Steps

Short term, consider reverting when a different currency is used and prices is not set. Long term, consider supporting multiple currencies in the NFT delegate.

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L480-L512

Vulnerability details

Impact

Project owner can mint NFTs without contributing, and these NFTs will still be eligible for redeeming a portion of an overflow. Since overflow is shared among all contributors, users who actually contributed will get a smaller share of the overflow. Another exploit scenario this issue can lead to is emission of voting power to an address controlled by project owner, since every NFT give its owner some amount of votes that can be used in government voting.

Proof of Concept

Overflow is the excessive amount of contributed assets that wasn't distributed and is left in project's treasury. Project contributors can also redeem a portion of an overflow, which is proportional to their contribution:

By default, overflow also serves a means for allowing community members to exit with a portion of the treasury's funds in hand. Any funds in overflow are reclaimable by the project's community by redeeming community tokens along a bonding curve defined by the project's current redemption rate.

In the case of NFT rewards, the total contribution amount of a contributor is the sum of contribution floors of all NFTs that they're holding (JBTiered721DelegateStore.sol#L514-L540):

/**
  @notice
  The cumulative weight the given token IDs have in redemptions compared to the `totalRedemptionWeight`.

  @param _nft The NFT for which the redemption weight is being calculated.
  @param _tokenIds The IDs of the tokens to get the cumulative redemption weight of.

  @return weight The weight.
*/
function redemptionWeightOf(address _nft, uint256[] calldata _tokenIds)
  public
  view
  override
  returns (uint256 weight)
{
  // Get a reference to the total number of tokens.
  uint256 _numberOfTokenIds = _tokenIds.length;

  // Add each token's tier's contribution floor to the weight.
  for (uint256 _i; _i < _numberOfTokenIds; ) {
    weight += _storedTierOf[_nft][tierIdOfToken(_tokenIds[_i])].contributionFloor;

    unchecked {
      ++_i;
    }
  }
}

The contribution floor of an NFT is in fact it's price (JBTiered721DelegateStore.sol#L1055-L1056):

// Make sure the amount meets the tier's contribution floor.
if (_storedTier.contributionFloor > leftoverAmount) revert INSUFFICIENT_AMOUNT();

Thus, contributors have to pay as little as the contribution floor of an NFT. However, project owner is allowed to mint NFTs to an arbitrary address without paying (JBTiered721Delegate.sol#L480-L512):

function mintFor(uint16[] memory _tierIds, address _beneficiary)
  public
  override
  onlyOwner
  returns (uint256[] memory tokenIds)
{
  // Record the mint. The returned token IDs correspond to the tiers passed in.
  (tokenIds, ) = store.recordMint( // @audit minting without a payment
    type(uint256).max, // force the mint. // @audit maximal payment amount is assumed
    _tierIds,
    true // manual mint
  );

  // Keep a reference to the number of tokens being minted.
  uint256 _numberOfTokens = _tierIds.length;

  // Keep a reference to the token ID being iterated on.
  uint256 _tokenId;

  for (uint256 _i; _i < _numberOfTokens; ) {
    // Set the token ID.
    _tokenId = tokenIds[_i];

    // Mint the token.
    _mint(_beneficiary, _tokenId);

    emit Mint(_tokenId, _tierIds[_i], _beneficiary, 0, msg.sender);

    unchecked {
      ++_i;
    }
  }
}

Such NFTs:

1. are eligible for a portion of an overflow since all NFTs in a tier have a contribution floor;
2. give its owner voting power.

Thus, a project owner can abuse this feature to get a portion of an overflow, or a higher voting power in a DAO.

Tools Used

Manual review

Recommended Mitigation Steps

Consider disallowing project owners to mint NFTs without contributing. If this is still required, consider minting NFTs with limited functionality (i.e. without the right for a redemption and without voting power) in the mintFor function.

Optimize NFT delegate deployments by using proxy

Targets

• https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateDeployer.sol#L115

Impact

The cost of NFT delegate deployments can be significantly reduced by deploying proxies instead of clones of the implementation.

Proof of Concept

This function is used to deploy new NFT delegates (JBTiered721DelegateDeployer.sol#L115):

function _clone(address _targetAddress) internal returns (address _out) {
  assembly {
    // Get deployed/runtime code size
    let _codeSize := extcodesize(_targetAddress)

    // Get a bit of freemem to land the bytecode, not updated as we'll leave this scope right after create(..)
    let _freeMem := mload(0x40)

    // Shift the length to the length placeholder, in the constructor
    let _mask := mul(_codeSize, 0x100000000000000000000000000000000000000000000000000000000)

    // Insert the length in the correct sport (after the PUSH3 / 0x62)
    let _initCode := or(_mask, 0x62000000600081600d8239f3fe00000000000000000000000000000000000000)

    // Store the deployment bytecode
    mstore(_freeMem, _initCode)

    // Copy the bytecode (our initialise part is 13 bytes long)
    extcodecopy(_targetAddress, add(_freeMem, 13), 0, _codeSize)

    // Deploy the copied bytecode
    _out := create(0, _freeMem, _codeSize)
  }

It copies the code of an existing contract (JBTiered721Delegate, JB721TieredGovernance, or JB721GlobalGovernance) and deploys a new contract with the same code. This is a costly operation because each of the three contracts is a big contract with a lot of code. It'll be much cheaper to deploy non-upgradable proxies instead.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider using the Clones library from OpenZeppelin–it deploys and absolutely minimal non-upgradable proxy contract. Such proxies, however, cannot be verified on Etherscan. Some more info.