Delegate architecture forces users to set zero slippage

[code423n4]

Lines of code

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

Vulnerability details

Delegate architecture forces users to set zero slippage

The design of the delegate forces users to set a zero value for the _minReturnedTokens parameter when calling pay() in the terminal.

Technical details

In order to implement the swap functionality, the JBXBuybackDelegate needs to signal the terminal to not mint any tokens when the swap path is token. This done by setting the weight variable to zero:

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

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);
151: 
152:         // Unpack the quote from the pool, given by the frontend
153:         (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
154: 
155:         // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
156:         if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
157:             // Pass the quote and reserve rate via a mutex
158:             mintedAmount = _tokenCount;
159:             reservedRate = _data.reservedRate;
160: 
161:             // Return this delegate as the one to use, and do not mint from the terminal
162:             delegateAllocations = new JBPayDelegateAllocation[](1);
163:             delegateAllocations[0] =
164:                 JBPayDelegateAllocation({delegate: IJBPayDelegate(this), amount: _data.amount.value});
165: 
166:             return (0, _data.memo, delegateAllocations);
167:         }
168: 
169:         // If minting, do not use this as delegate
170:         return (_data.weight, _data.memo, new JBPayDelegateAllocation[](0));
171:     }

This can be seen in line 166 in the previous snippet of code, where the function returns zero as the weight return value when going the swap path. This weight is then used to calculate the tokenCount in the JBSingleTokenPaymentTerminalStore3_1 contract:

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

...
tokenCount = PRBMath.mulDiv(_amount.value, _weight, _weightRatio);
...

This tokenCount variable is finally returned to the JBPayoutRedemptionPaymentTerminal3_1 contract, which uses it to calculate the amount of tokens to mint and validate the tokens assigned to the beneficiary are above the minimum:

https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1470-L1493

...

(_fundingCycle, _tokenCount, _delegateAllocations, _memo) = store.recordPaymentFrom(
  _payer,
  _bundledAmount,
  _projectId,
  baseWeightCurrency,
  _beneficiary,
  _memo,
  _metadata
);

// Mint the tokens if needed.
if (_tokenCount > 0)
  // Set token count to be the number of tokens minted for the beneficiary instead of the total amount.
  beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(
    _projectId,
    _tokenCount,
    _beneficiary,
    '',
    _preferClaimedTokens,
    true
  );

// The token count for the beneficiary must be greater than or equal to the minimum expected.
if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT();

...

This means that if the swap path is chosen, weight is going to be zero, which sets _tokenCount to zero, and implies that beneficiaryTokenCount is going to be zero as well. Setting _minReturnedTokens to any value different than zero will make the transaction revert.

However, the "buyback" path is not the only alternative. The delegate could also take the "vanilla" path in which it simply bypasses the delegate and tokens are minted in the terminal (line 170 in payParams()). This creates a conflict between both paths that forces the user to set _minReturnedTokens to zero, because the taken path is resolved at transaction time. Setting _minReturnedTokens to any value different than zero will make the "buyback" path revert, while setting _minReturnedTokens to zero nullifies the slippage check when the "vanilla" path is taken.

Impact

Medium. The current design forces the user to set _minReturnedTokens to zero and disables any type of check over the amount of minted tokens.

Recommendation

It is difficult to give a recommendation that doesn't involve significant changes to the code, as the current design relies on returning a zero weight when choosing the "buyback" path, which forces the condition in _minReturnedTokens in order to prevent the transaction revert. Nevertheless, the situation should be revised, as setting a zero value for _minReturnedTokens can have a significant impact on users engaging with the protocol.

Assessed type

Other

[c4-pre-sort]

dmvt marked the issue as duplicate of #36

[c4-judge]

dmvt marked the issue as selected for report

[dmvt]

See sponsor comments and my responses on #36