More outstanding reserved tokens are distributed than anticipated leading to less redeemable assets and therefore loss of user funds

[code423n4]

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

[mejango]

The only case where the tracker can be -1000 but the totalEligibleTokens is 0 is if reserved rate is 100%. https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L664

[mejango]

Furthermore, reserved rate changes per fc is noted in the protocol's known risks exposed by design:: https://info.juicebox.money/dev/learn/risks#undistributed-reserved-rate-risk

[jack-the-pug]

I find this issue to be a valid Medium issue as it introduced an unexpected behavior that can cause a leak of value in certain circumstances.