Delegate uses incorrect weight ratio to calculate token count in `payParams()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L150

Vulnerability details

Delegate uses incorrect weight ratio to calculate token count in payParams()

The JBXBuybackDelegate assumes a hardcoded weight ratio of 10 ** 18 while calculating the token amount and may yield incorrect results leading to a potential loss of value.

Impact

The payParams() function present in the buyback delegate calculates the amount of tokens based on the passed weight in order to compare it against the Uniswap pool quote, and decide between the "buyback" path or the "vanilla" path.

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L144-L150

144:     function payParams(JBPayParamsData calldata _data)
145:         external
146:         override
147:         returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
148:     {
149:         // Find the total number of tokens to mint, as a fixed point number with 18 decimals
150:         uint256 _tokenCount = PRBMath.mulDiv(_data.amount.value, _data.weight, 10 ** 18);

As we can see in the previous snippet of code, line 150 simply hardcodes the weight ratio (the denominator in the mulDiv operation) to 10 ** 18. This assumption and simplification is wrong, since the weight ratio depends on different factors, as we can see in the implementation of JBSingleTokenPaymentTerminalStore3_1:

https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/JBSingleTokenPaymentTerminalStore3_1.sol#L420-L427

  ...

  // If the terminal should base its weight on a different currency from the terminal's currency, determine the factor.
  // The weight is always a fixed point mumber with 18 decimals. To ensure this, the ratio should use the same number of decimals as the `_amount`.
  uint256 _weightRatio = _amount.currency == _baseWeightCurrency
    ? 10**_decimals
    : prices.priceFor(_amount.currency, _baseWeightCurrency, _decimals);

  // Find the number of tokens to mint, as a fixed point number with as many decimals as `weight` has.
  tokenCount = PRBMath.mulDiv(_amount.value, _weight, _weightRatio);
}

This will severely impact the operation and potentially yield an undesirable or non-optimal amount of tokens, rendering the delegate intentionality useless. Let's consider both cases:

• If the weight ratio is higher than it should be, the token count will be less than expected (as this factor is dividing the calculation), which may lead the delegate to choose the "buyback" path and execute the swap, where in reality the "vanilla" path would mint more tokens and provide better results for the user.
• On the other hand, if the weight ratio is lower than it should be, the token count calculation will be higher than expected, which makes the delegate go for the "vanilla" path, where in reality the swap could be a better option.

Recommendation

The _tokenCount calculation present in the payParams() function of the delegate should use the same logic as the tokenCount calculation present in the recordPaymentFrom() function of the JBSingleTokenPaymentTerminalStore3_1 contract.

Assessed type

Math

[c4-pre-sort]

dmvt marked the issue as duplicate of #92

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b