Juicebox contest - berndartmueller's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L1224-L1259 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateStore.sol#L566 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/abstract/JB721Delegate.sol#L142

Vulnerability details

If the reserved rate of a tier is set to a value > JBConstants.MAX_RESERVED_RATE, the JBTiered721DelegateStore._numberOfReservedTokensOutstandingFor function will return way more outstanding reserved tokens (up to ~6 times more than allowed - 2^16 - 1 due to the manual cast of reservedRate to uint16 divided by JBConstants.MAX_RESERVED_RATE = 10_000). This inflated value is used in the JBTiered721DelegateStore.totalRedemptionWeight function to calculate the cumulative redemption weight of all tokens across all tiers.

This higher-than-expected redemption weight will lower the reclaimAmount calculated in the JB721Delegate.redeemParams function. Depending on the values of _data.overflow and _redemptionWeight, the calculated reclaimAmount can be 0 (due to rounding down, see here) or a smaller than anticipated value, leading to burned NFT tokens from the user and no redemptions.

Impact

The owner of an NFT contract can add tiers with higher than usual reserved rates (and mint an appropriate number of NFTs to bypass all conditions in the JBTiered721DelegateStore._numberOfReservedTokensOutstandingFor), which will lead to a lower-than-expected redemption amount for users.

Proof of Concept

JBTiered721DelegateStore._numberOfReservedTokensOutstandingFor

function _numberOfReservedTokensOutstandingFor(
  address _nft,
  uint256 _tierId,
  JBStored721Tier memory _storedTier
) internal view returns (uint256) {
  // Invalid tier or no reserved rate?
  if (_storedTier.initialQuantity == 0 || _storedTier.reservedRate == 0) return 0;

  // No token minted yet? Round up to 1.
  if (_storedTier.initialQuantity == _storedTier.remainingQuantity) return 1;

  // The number of reserved tokens of the tier already minted.
  uint256 _reserveTokensMinted = numberOfReservesMintedFor[_nft][_tierId];

  // If only the reserved token (from the rounding up) has been minted so far, return 0.
  if (_storedTier.initialQuantity - _reserveTokensMinted == _storedTier.remainingQuantity)
    return 0;

  // Get a reference to the number of tokens already minted in the tier, not counting reserves or burned tokens.
  uint256 _numberOfNonReservesMinted = _storedTier.initialQuantity -
    _storedTier.remainingQuantity -
    _reserveTokensMinted;

  // Store the numerator common to the next two calculations.
  uint256 _numerator = uint256(_numberOfNonReservesMinted * _storedTier.reservedRate);

  // Get the number of reserved tokens mintable given the number of non reserved tokens minted. This will round down.
  uint256 _numberReservedTokensMintable = _numerator / JBConstants.MAX_RESERVED_RATE;

  // Round up.
  if (_numerator - JBConstants.MAX_RESERVED_RATE * _numberReservedTokensMintable > 0)
    ++_numberReservedTokensMintable;

  // Return the difference between the amount mintable and the amount already minted.
  return _numberReservedTokensMintable - _reserveTokensMinted;
}

JBTiered721DelegateStore.totalRedemptionWeight

The JBTiered721DelegateStore._numberOfReservedTokensOutstandingFor function is called from within the JBTiered721DelegateStore.totalRedemptionWeight function. This allows for inflating the total redemption weight.

function totalRedemptionWeight(address _nft) public view override returns (uint256 weight) {
  // Keep a reference to the greatest tier ID.
  uint256 _maxTierId = maxTierIdOf[_nft];

  // Keep a reference to the tier being iterated on.
  JBStored721Tier memory _storedTier;

  // Add each token's tier's contribution floor to the weight.
  for (uint256 _i; _i < _maxTierId; ) {
    // Keep a reference to the stored tier.
    _storedTier = _storedTierOf[_nft][_i + 1];

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

    unchecked {
      ++_i;
    }
  }
}

JBTiered721Delegate._totalRedemptionWeight

JBTiered721DelegateStore.totalRedemptionWeight is called in the JBTiered721Delegate._totalRedemptionWeight function.

function _totalRedemptionWeight() internal view virtual override returns (uint256) {
  return store.totalRedemptionWeight(address(this));
}

abstract/JB721Delegate.redeemParams

This JBTiered721Delegate._totalRedemptionWeight function is then called in the JB721Delegate.redeemParams function, which ultimately calculates the reclaimAmount given an overflow and _decodedTokenIds.

uint256 _base = PRBMath.mulDiv(_data.overflow, _redemptionWeight, _total); in line 142 will lead to a lower _base due to the inflated denumerator _total.

function redeemParams(JBRedeemParamsData calldata _data)
  external
  view
  override
  returns (
    uint256 reclaimAmount,
    string memory memo,
    JBRedemptionDelegateAllocation[] memory delegateAllocations
  )
{
  // Make sure fungible project tokens aren't being redeemed too.
  if (_data.tokenCount > 0) revert UNEXPECTED_TOKEN_REDEEMED();

  // Check the 4 bytes interfaceId and handle the case where the metadata was not intended for this contract
  if (
    _data.metadata.length < 4 || bytes4(_data.metadata[0:4]) != type(IJB721Delegate).interfaceId
  ) {
    revert INVALID_REDEMPTION_METADATA();
  }

  // Set the only delegate allocation to be a callback to this contract.
  delegateAllocations = new JBRedemptionDelegateAllocation[](1);
  delegateAllocations[0] = JBRedemptionDelegateAllocation(this, 0);

  // If redemption rate is 0, nothing can be reclaimed from the treasury
  if (_data.redemptionRate == 0) return (0, _data.memo, delegateAllocations);

  // Decode the metadata
  (, uint256[] memory _decodedTokenIds) = abi.decode(_data.metadata, (bytes4, uint256[]));

  // Get a reference to the redemption rate of the provided tokens.
  uint256 _redemptionWeight = _redemptionWeightOf(_decodedTokenIds);

  // Get a reference to the total redemption weight.
  uint256 _total = _totalRedemptionWeight(); // @audit-info Uses the inflated total redemption weight

  // Get a reference to the linear proportion.
  uint256 _base = PRBMath.mulDiv(_data.overflow, _redemptionWeight, _total);

  // These conditions are all part of the same curve. Edge conditions are separated because fewer operation are necessary.
  if (_data.redemptionRate == JBConstants.MAX_REDEMPTION_RATE)
    return (_base, _data.memo, delegateAllocations);

  // Return the weighted overflow, and this contract as the delegate so that tokens can be deleted.
  return (
    PRBMath.mulDiv(
      _base,
      _data.redemptionRate +
        PRBMath.mulDiv(
          _redemptionWeight,
          JBConstants.MAX_REDEMPTION_RATE - _data.redemptionRate,
          _total
        ),
      JBConstants.MAX_REDEMPTION_RATE
    ),
    _data.memo,
    delegateAllocations
  );
}

Tools Used

Manual review

Recommended mitigation steps

Consider validating the tier reserved rate reservedRate in the JBTiered721DelegateStore.recordAddTiers function to ensure the reserved rate is not greater than JBConstants.MAX_RESERVED_RATE.

Table of Contents

• Low Risk
  • [L-01] JBTiered721Delegate.tokenURI should throw an error if _tokenId is not a valid NFT
  • [L-02] Decoding an IPFS hash is using a fixed hash function and length of hash
  • [L-03] The tier id can potentially surpass 16 bits leading to token id collisions

Low Risk

[L-01] JBTiered721Delegate.tokenURI should throw an error if _tokenId is not a valid NFT

Description

According to EIP-721 and specifically, the metadata extension, the tokenURI function should throw an error if _tokenId is not a valid NFT. Contrary, the current implementation returns an empty string.

Findings

JBTiered721Delegate.sol#L140

function tokenURI(uint256 _tokenId) public view override returns (string memory) {
  // A token without an owner doesn't have a URI.
  if (_owners[_tokenId] == address(0)) return ''; // @audit-info Should throw instead of returning an empty string

  // Get a reference to the URI resolver.
  IJBTokenUriResolver _resolver = store.tokenUriResolverOf(address(this));

  // If a token URI resolver is provided, use it to resolve the token URI.
  if (address(_resolver) != address(0)) return _resolver.getUri(_tokenId);

  // Return the token URI for the token's tier.
  return
    JBIpfsDecoder.decode(
      store.baseUriOf(address(this)),
      store.encodedTierIPFSUriOf(address(this), _tokenId)
    );
}

Recommended mitigation steps

Consider throwing an error if _tokenId is not a valid NFT.

[L-02] Decoding an IPFS hash using a fixed hash function and length of the hash

Description

An IPFS hash specifies the hash function and length of the hash in the first two bytes of the hash. The first two bytes are 0x1220, where 12 denotes that this is the SHA256 hash function and 20 is the length of the hash in bytes (32 bytes).

Although SHA256 is 32 bytes and is currently the most common IPFS hash function, other content could use a hash function that is larger than 32 bytes. The current implementation limits the usage to the SHA256 hash function and a hash length of 32 bytes.

Findings

libraries/JBIpfsDecoder.sol#L28

function decode(string memory _baseUri, bytes32 _hexString)
  external
  pure
  returns (string memory)
{
  // Concatenate the hex string with the fixed IPFS hash part (0x12 and 0x20)
  bytes memory completeHexString = abi.encodePacked(bytes2(0x1220), _hexString);

  // Convert the hex string to an hash
  string memory ipfsHash = _toBase58(completeHexString);

  // Concatenate with the base URI
  return string(abi.encodePacked(_baseUri, ipfsHash));
}

Recommended mitigation steps

Consider using a more generic implementation that can handle different hash functions and lengths and allow the user to choose.

[L-03] The tier id can potentially surpass 16 bits leading to token id collisions

Description

The token id is composed of the given tier id _tierId and the number of the token _tokenNumber in the tier. The tier id is limited to 16 bits, which means that there can theoretically only exist 65,535 tiers (this is very unlikely as this would have more serious consequences on other parts of the system and will cause a serious denial of service caused by unbounded loops. Still, theoretically, it's possible and there is no check in place).

If more than 65,535 tiers exist, the 16 bits reserved for the tier id will be surpassed and overwritten by _tokenNumber. This will lead to token id collisions with other tiers with a lower tier id.

Findings

JBTiered721DelegateStore._generateTokenId

function _generateTokenId(uint256 _tierId, uint256 _tokenNumber)
  internal
  pure
  returns (uint256 tokenId)
{
  // The tier ID in the first 16 bits.
  tokenId = _tierId;

  // The token number in the rest.
  tokenId |= _tokenNumber << 16;
}

Recommended mitigation steps

Consider reverting if the _tierId is > 16 bits.

Table of Contents

• [G-01] Tier removal check is not necessary in JBTiered721DelegateStore.recordAddTiers

[G-01] Tier removal check is not necessary in JBTiered721DelegateStore.recordAddTiers

Description

A tier is considered removed if the bit in the _isTierRemoved bitmap is set accordingly. The function JBTiered721DelegateStore.recordAddTiers adds new tiers and sorts them by contributionFloor, no matter if a tier is removed or not. Therefore, the _isTierRemoved bitmap reads are unnecessary and can be safely removed.

Findings

JBTiered721DelegateStore.sol#L724-L725
JBTiered721DelegateStore.sol#L717

Recommended mitigation steps

Consider removing the _isTierRemoved bitmap reads.