Juicebox contest - Trust's results

Lines of code

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

Vulnerability details

Description

This is another bug in the redemption weight calculation mechanism discussed in an issue with the same title. I recommend to read that first for context.

Let's look at DelegateStore's totalRedemptionWeight implementation again:

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

We can see that the reservedTokensOutstanding is added to the product of contribution * minted tokens. This is a severe mistake, because the weight is meant to take into account the specific tier of the outstanding tokens. Currently all reserved tokens are added linearly to the result and will be negligible if contributionFloor is much larger than 1.

Note that after the reserved tokens are minted, the reservedTokensOutstanding and remainingQuantity count will be  decremented, making the minted tokens multiply by contributionFloor. Therefore, we have conflicting weights while no money has been put in.

The impact of this bug is incorrect calculation of the weight of user's contributions. While reserve tokens are yet to be minted, users can redeem tokens and receive much more than they should have, at the expense of the reserved token beneficiary.

Impact

Project reserve tokens lose value when users redeem their NFTs before reserves were minted. 

Proof of Concept

1. Suppose we have a project with 2 tiers, reserve ratio = 50%, redemption ratio = 100%:

When calculating totalRedemptionWeight(), the correct result is

50 * (10 - 4 + 3) = 450

The wrong result will be:

50 * (10 - 4) + 3  = 303

Therefore, when users redeem NFT rewards, they will get far more value than they are eligible for. This means eventually when the reserve tokens will be minted, they will be worth much less than they are supposed to.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the calculation to:

_storedTier.contributionFloor *
      (_storedTier.initialQuantity - _storedTier.remainingQuantity + 
      _numberOfReservedTokensOutstandingFor(_nft, _i + 1, _storedTier) )

Lines of code

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

Vulnerability details

Description

When the project wishes to mint reserved tokens, they call mintReservesFor which allows minting up to the amount calculated by DelegateStore's _numberOfReservedTokensOutstandingFor. The function has this line:

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

In order to ease calculations, if reserve rate is not 0 and no token has been minted yet, the function allows a single reserve token to be printed. It turns out that this introduces a very significant risk for users. Projects can launch with several tierIDs of similar contribution size, and reserve rate as low as 1%. Once a victim contributes to the project, it can instantly mint a single reserve token of all the rest of the tiers. They can then redeem the reserve token and receive most of the user's contribution, without putting in any money of their own.

Since this attack does not require setting "dangerous" flags like lockReservedTokenChanges or lockManualMintingChanges, it represents a very considerable threat to unsuspecting users. Note that the attack circumvents user voting or any funding cycle changes which leave time for victim to withdraw their funds. 

Impact

Honeypot project can instantly take most of first user's contribution.

Proof of Concept

New project launches, with 10 tiers, of contributions 1000, 1050, 1100, ...

Reserve rate is set to 1% and redemption rate = 100%

User contributes 1100 and gets a Tier 3 NFT reward. 

Project immediately mints Tier 1,  Tier 2, Tier 4,... Tier 10 reserve tokens, and redeems all the reserve tokens.

Project's total weight = 12250

Reserve token weight = 11150

Malicious project cashes 1100 (overflow) * 11150 / 12250 = ~1001 tokens.

Tools Used

Manual audit

Recommended Mitigation Steps

Don't round up outstanding reserve tokens as it represents too much of a threat.

Lines of code

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

Vulnerability details

Description

Redemption weight is a concept used in Juicebox to determine investor's eligible percentage of the non-locked funds. In redeemParams, JB721Delegate calculates user's share using:

uint256 _redemptionWeight = _redemptionWeightOf(_decodedTokenIds);
uint256 _total = _totalRedemptionWeight();
uint256 _base = PRBMath.mulDiv(_data.overflow, _redemptionWeight, _total);

_totalRedemptionWeight eventually is implemented in DelegateStore:

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

If we pay attention to _numberOfReservedTokensOutstandingFor() call, we can see it is called with tierId = i, yet storedTier of i+1. It is definitely not the intention as for example, recordMintReservesFor() uses the function correctly:

function recordMintReservesFor(uint256 _tierId, uint256 _count)
  external
  override
  returns (uint256[] memory tokenIds)
{
  // Get a reference to the tier.
  JBStored721Tier storage _storedTier = _storedTierOf[msg.sender][_tierId];
  // Get a reference to the number of reserved tokens mintable for the tier.
  uint256 _numberOfReservedTokensOutstanding = _numberOfReservedTokensOutstandingFor(
    msg.sender,
    _tierId,
    _storedTier
  );
  ...

The impact of this bug is incorrect calculation of the weight of user's contributions. The initialQuantity and remainingQuantity values are taken from the correct tier, but _reserveTokensMinted minted is taken from previous tier. In the case where _reserveTokensMinted is smaller than correct value, for example tierID=0 which is empty, the outstanding value returned is larger, meaning weight is larger and redemptions are worth less. In the opposite case, where lower tierID has higher _reserveTokensMinted, the redemptions will receive more payout than they should.

Impact

Users of projects can receive less or more funds than they are eligible for when redeeming NFT rewards.

Proof of Concept

1. Suppose we have a project with 2 tiers, reserve ratio = 50%, redemption ratio = 100%:

When calculating totalRedemptionWeight(), the correct result is

50 * (10 - 3) + 2 + 100 * (30-2) + 2 = 3154

The wrong result will be:

50 * (10 -3) + 4 + 100 * (30-2) + 13  = 3167

Therefore, when users redeem NFT rewards, they will get less value than they are eligible for. Note that totalRedemptionWeight() has an additional bug where the reserve amount is not multiplied by the contribution, which is discussed in another submission. If it would be calculated correctly, the correct weight would be 3450.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the calculation to:

_numberOfReservedTokensOutstandingFor(_nft, _i+1, _storedTier);

Additional discussion

Likelihood of impact is very high, because the conditions will arise naturally (different tiers, different reserve minted count for each tier, user calls redeem).  Severity of impact is high because users receive less or more tokens than they are eligible for.

Initially I thought this bug could allow attacker to steal entire unlocked project funds, using a mint/burn loop. However, this would not be profitable because their calculated share of the funds would always be at most what they put in, because reserve tokens are printed out of thin air.

Lines of code

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

Vulnerability details

Description

Tiers in Juicebox can be deactivated using the adjustTiers() function. It makes sense that reserve tokens may be minted in deactivated tiers, in order to be consistent with already minted tokens. However, the code allows the first reserve token to be minted in a deactivated tier, even though there was no previous minting of that tier.

function recordMintReservesFor(uint256 _tierId, uint256 _count)
  external
  override
  returns (uint256[] memory tokenIds)
{
  // Get a reference to the tier.
  JBStored721Tier storage _storedTier = _storedTierOf[msg.sender][_tierId];
  // Get a reference to the number of reserved tokens mintable for the tier.
  uint256 _numberOfReservedTokensOutstanding = _numberOfReservedTokensOutstandingFor(
    msg.sender,
    _tierId,
    _storedTier
  );
  if (_count > _numberOfReservedTokensOutstanding) revert INSUFFICIENT_RESERVES();
  ...
  for (uint256 _i; _i < _count; ) {
  // Generate the tokens.
  tokenIds[_i] = _generateTokenId(
    _tierId,
    _storedTier.initialQuantity - --_storedTier.remainingQuantity + _numberOfBurnedFromTier
  );
  unchecked {
    ++_i;
  }
}

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.
  // ******************* BUG HERE *********************
  if (_storedTier.initialQuantity == _storedTier.remainingQuantity) return 1;
  ...
}

Using the rounding mechanism is not valid when the tier has been deactivated, since we know there won't be any minting of this tier.

Impact

The reserve beneficiary receives an unfair NFT which may be used to withdraw tokens using the redemption mechanism.

Tools Used

Manual audit

Recommended Mitigation Steps

If Juicebox intends to use rounding functionality, pass an argument isDeactivated which, if true, deactivated the rounding logic.

Issues

1. _clone() in JBTiered721DelegateDeplyer.sol trims last 13 bytes of deployed contract code

The code in _clone() inserts a tiny initialization code before the deployed bytecode. It uses the deployed bytecode size in the create() call, which expects initialization bytecode size. This means 13 bytes, the initialization bytecode size, are trimmed from the source contract when copied to the target contract. These bytes contain compiler metadata as discussed here . The consequence is that contract verifiers like etherscan will fail to verify the source code of project delegates.