payParams function prevents users from specifying minReturnedTokens, when using the swap pathway

[code423n4]

Lines of code

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

Vulnerability details

Impact

The payParams function will cause the pay() function in the terminal (used to call in the JBXBuybackDelegate contract) to revert, if the user specify a minimum amount of token to mint (other than 0), while the swap pathway is selected. Preventing the user from being able to specify the minimum amount of tokens to receive.
(They can still specify the splippage in the swap, but didPay(), will mint the tokens instead if such criteria is not met https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L205).

Proof of Concept

One of the values returned by payParams will be set to 0 each time the swap pathway is taken.
This value will later cause the following statement to revert: if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT(); (in the pay() function, from the terminal contract) for any value of _minReturnedTokens !=0. More in detail:

"If the amount swapped is bigger than the lowest received when minting" , the payParams function will returun the following values:

return (0, _data.memo, delegateAllocations);

(As can be seen here: https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L156 ,
https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L166 ).

Those values (the 0 in particular) are used by the recordPaymentFrom() in the JBSingleTokenPaymentTerminalStore3_1.sol contract. Which calls payParams to set _weight:

(_weight, memo, delegateAllocations) = IJBFundingCycleDataSource(fundingCycle.dataSource()).payParams(_data);

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/JBSingleTokenPaymentTerminalStore3_1.sol#L370)

and than uses the result to decide what to return:

if (_weight == 0) return (fundingCycle, 0, delegateAllocations, memo);

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/JBSingleTokenPaymentTerminalStore3_1.sol#L415).

This operations are invoked by the pay() function of the JBPayoutRedemptionPaymentTerminal3_1 contract, to set the following parameters:

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

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1470).

As you might see from the calls above, when the swap pathway is taken _tokenCount in the previous call will be set to 0.
In the instructions right below this, the pay function will check if _tokenCount is greater than 0, to decide weather or not it should mint some tokens to the beneficiary:

     if (_tokenCount > 0) beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(
= _projectId, _tokenCount, _beneficiary,'',_preferClaimedTokens,true);

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1481)

Since _tokenCount == 0, beneficiaryTokenCount will keep it's uninitialized vale of 0 and the check right bellow will fail for any value of _minReturnedTokens diffrent from 0:

if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT();

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1493)

_minReturnedTokens being the value passed to the pay() function to specify the minimum amount of tokens to mint.
(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1437)

Here is a Forgery test that you can run. It checks for a revert with error: INADEQUATE_TOKEN_COUNT (on line 171):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.16;

import '../interfaces/external/IWETH9.sol';
import './helpers/TestBaseWorkflowV3.sol';

import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBController3_1.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleStore.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleBallot.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleDataSource.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBOperatable.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBPayDelegate.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBRedemptionDelegate.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBPayoutRedemptionPaymentTerminal.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBSingleTokenPaymentTerminalStore.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBToken.sol';

import '@jbx-protocol/juice-contracts-v3/contracts/libraries/JBConstants.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/libraries/JBCurrencies.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/libraries/JBFundingCycleMetadataResolver.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/libraries/JBOperations.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/libraries/JBTokens.sol';
import '@jbx-protocol/juice-contracts-v3/contracts/structs/JBFundingCycle.sol';

import '@paulrberg/contracts/math/PRBMath.sol';
import '@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol';
import '@uniswap/v3-core/contracts/interfaces/IUniswapV3Factory.sol';
import '@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback.sol';
import '@uniswap/v3-periphery/contracts/interfaces/INonfungiblePositionManager.sol';
import '@uniswap/v3-core/contracts/libraries/TickMath.sol';

import '@exhausted-pigeon/uniswap-v3-forge-quoter/src/UniswapV3ForgeQuoter.sol';

import '../JBXBuybackDelegate.sol';
import '../mock/MockAllocator.sol';

import 'forge-std/Test.sol';

import{JBPayoutRedemptionPaymentTerminal3_1} from 'node_modules/@jbx-protocol/juice-contracts-v3/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol';

/**
 * @notice JBXBuyback fork integration tests, using $jbx v3
 */
contract TestIntegrationJBXBuybackDelegate is Test, UniswapV3ForgeQuoter {
  using JBFundingCycleMetadataResolver for JBFundingCycle;

  event JBXBuybackDelegate_Swap(uint256 projectId, uint256 amountEth, uint256 amountOut);
  event JBXBuybackDelegate_Mint(uint256 projectId);
  event Mint(
    address indexed holder,
    uint256 indexed projectId,
    uint256 amount,
    bool tokensWereClaimed,
    bool preferClaimedTokens,
    address caller
  );

  // Contracts needed
  IJBFundingCycleStore jbFundingCycleStore;
  IJBProjects jbProjects;
  IJBSplitsStore jbSplitsStore;
  IJBPayoutRedemptionPaymentTerminal3_1 jbEthPaymentTerminal;
  IJBSingleTokenPaymentTerminalStore jbTerminalStore;
  IJBController3_1 jbController;
  IJBTokenStore jbTokenStore;

  // Structure needed
  JBProjectMetadata projectMetadata;
  JBFundingCycleData data;
  JBFundingCycleMetadata metadata;
  JBFundAccessConstraints[] fundAccessConstraints;
  IJBPaymentTerminal[] terminals;
  JBGroupedSplits[] groupedSplits;

  JBXBuybackDelegate delegate;

  IUniswapV3Pool pool;

  IERC20 jbx = IERC20(0x4554CC10898f92D45378b98D6D6c2dD54c687Fb2);  // 0 - 69420*10**18
  IWETH9 weth = IWETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); // 1 - 1*10**18

  uint256 price = 69420 ether;
  
  // sqrtPriceX96 = sqrt(1*10**18 << 192 / 69420*10**18) = 300702666377442711115399168 (?)
  uint160 sqrtPriceX96 = 300702666377442711115399168;

  uint256 amountOutForOneEth;

  function setUp() public {
    vm.createSelectFork(vm.envString("RPC_MAINNET_URL"), 17239357);

    // Collect the mainnet deployment addresses
    jbEthPaymentTerminal = IJBPayoutRedemptionPaymentTerminal3_1(
        stdJson.readAddress(
            vm.readFile("node_modules/@jbx-protocol/juice-contracts-v3/deployments/mainnet/JBETHPaymentTerminal3_1.json"), ".address"
        )
    );
    vm.label(address(jbEthPaymentTerminal), "jbEthPaymentTerminal3_1");

    jbController = IJBController3_1(
        stdJson.readAddress(vm.readFile("node_modules/@jbx-protocol/juice-contracts-v3/deployments/mainnet/JBController3_1.json"), ".address")
    );
    vm.label(address(jbController), "jbController");

    jbTokenStore = jbController.tokenStore();
    jbFundingCycleStore = jbController.fundingCycleStore();
    jbProjects = jbController.projects();
    jbSplitsStore = jbController.splitsStore();

    pool = IUniswapV3Pool(IUniswapV3Factory(0x1F98431c8aD98523631AE4a59f267346ea31F984).createPool(address(weth), address(jbx), 100));
    pool.initialize(sqrtPriceX96); // 1 eth <=> 69420 jbx

    vm.startPrank(address(123), address(123));
    deal(address(weth), address(123), 10000000 ether);
    deal(address(jbx), address(123),  10000000 ether);
    
    // approve:
    address POSITION_MANAGER = 0xC36442b4a4522E871399CD717aBDD847Ab11FE88;
    jbx.approve(POSITION_MANAGER, 10000000 ether);
    weth.approve(POSITION_MANAGER, 10000000 ether);

    // mint concentrated position 
    INonfungiblePositionManager.MintParams memory params =
            INonfungiblePositionManager.MintParams({
                token0: address(jbx),
                token1: address(weth),
                fee: 100,
                tickLower: TickMath.getTickAtSqrtRatio(sqrtPriceX96) - 10 * pool.tickSpacing(),
                tickUpper: TickMath.getTickAtSqrtRatio(sqrtPriceX96) + 10 * pool.tickSpacing(),
                amount0Desired: 10000000 ether,
                amount1Desired: 10000000 ether,
                amount0Min: 0,
                amount1Min: 0,
                recipient: address(123),
                deadline: block.timestamp
            });

    INonfungiblePositionManager(POSITION_MANAGER).mint(params);

    vm.stopPrank();

    amountOutForOneEth = getAmountOut(pool, 1 ether, address(weth));

    delegate = new JBXBuybackDelegate(IERC20(address(jbx)), weth, pool, jbEthPaymentTerminal);

    vm.label(address(pool), 'uniswapPool');
    vm.label(address(weth), '$WETH');
    vm.label(address(jbx), '$JBX');
  }



//THIS TEST WILL PASS IF pay() REVERST WITH "INADEQUATE_TOKEN_COUNT"

  function test_swapIfQuoteBetter(uint256 _weight) public {
 
    _weight = bound(_weight, 0, amountOutForOneEth - (amountOutForOneEth * 500 / 10000) - 1);
    _reconfigure(1, address(delegate), _weight, 5000);

    uint256 _reservedBalanceBefore = jbController.reservedTokenBalanceOf(1);

    // Build the metadata using the quote at that block
    bytes memory _metadata = abi.encode(
      bytes32(0),
      bytes32(0),
      amountOutForOneEth, //quote
      500 //slippage 500/10000 = 5%
    );
    

    vm.expectRevert(JBPayoutRedemptionPaymentTerminal3_1.INADEQUATE_TOKEN_COUNT.selector);
    // Pay the project
    jbEthPaymentTerminal.pay{value: 1 ether}(
      1,
      1 ether,
      address(0),
      address(123),
      /* _minReturnedTokens */
      123,
      /* _preferClaimedTokens */
      true,
      /* _memo */
      'Take my money!',
      /* _delegateMetadata */
      _metadata
    );


  }

function _reconfigure(uint256 _projectId, address _delegate, uint256 _weight, uint256 _reservedRate) internal {
    address _projectOwner = jbProjects.ownerOf(_projectId);

    JBFundingCycle memory _fundingCycle = jbFundingCycleStore.currentOf(_projectId);
    metadata = _fundingCycle.expandMetadata();

    JBGroupedSplits[] memory _groupedSplits = new JBGroupedSplits[](1);
    _groupedSplits[0] = JBGroupedSplits({
        group: 1,
        splits: jbSplitsStore.splitsOf(
            _projectId,
            _fundingCycle.configuration, /*domain*/
            JBSplitsGroups.ETH_PAYOUT /*group*/)
    });

    metadata.useDataSourceForPay = true;
    metadata.dataSource = _delegate;

    metadata.reservedRate = _reservedRate;

    data.weight = _weight;
    data.duration = 14 days;

    // reconfigure
    vm.prank(_projectOwner);
    jbController.reconfigureFundingCyclesOf(
        _projectId, data, metadata, block.timestamp, _groupedSplits, fundAccessConstraints, ""
    );

    // Move to next fc
    vm.warp(block.timestamp + 14 days + 1);
  }
}

The test will fail when _minReturnedTokens is set to 0, and pass for the other values.

Recommended Mitigation Steps

The if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT(); check should only be performed when _tokenCount > 0.

Assessed type

Invalid Validation

[c4-pre-sort]

dmvt marked the issue as duplicate of #36

[c4-judge]

dmvt marked the issue as satisfactory