code423n4 commented 1 year ago

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%:


Tier	Contribution	Initial quantity	Remaining quantity	Reserves minted	Reserves outstanding
Tier 1	50	10	3	1	2
Tier 2	100	30	2	8	2

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.

c4-judge commented 1 year ago

Picodes marked the issue as selected for report