Attempting to swap a larger amount of tokens than what is available in the V3 pool, with didPay(), will result in loss of funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

If a user sends ether, to didPay(), to buy more tokens than what is available in the underlying UnicornV3Pool, the difference between the eth used to buy all the available tokens in the pool, and the amount sent, will be locked in the JBXBuybackDelegate contract. Resulting in a loss of funds for the user.
Since both to project's token and the V3 pool, are controlled by the owner of the project and/or other external factors, no assumptions on the price and/or availability of the token can be made. Therefor an estimation on the amount of eth necessary for this to happen, or on the likelyhood in general, cannot be made.

Proof of Concept

The didPay(JBDidPayData calldata _data) functions is invoked by the JBPayoutRedemptionPaymentTerminal3_1 with the following line of code:

_delegateAllocation.delegate.didPay{value: _payableValue}(_data);

(https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1532).
Where _payableValue is the eth being send with the pay() function by the user, to pay for the tokens.
If didPay ends up calling into the _mint() function available in the same contract. This eth will be sent back to the terminal as payment for minting the tokens.
If didPay performs a swap in the V3 pool to acquire the projectTokens, the uniswapV3SwapCallback function will convert the eth requested by the pool, into wEth, and than transfers it as payment. (https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L231). No other actions will be performed on the value sent by the user along with the pay() request. Therefor any eth unused by the uniswapV3SwapCallback function will be locked in the contract.
If the amount of token available in the pool is lesser than the amount requested, uniswap will send all the available tokens to the caller. And the uniswapV3SwapCallback, will only spend the amount of eth necessary to pay for them. The difference between what is being paid for the swap and what has been sent, will be locked in the contract.

Here is a foundry test that you can run. It will check weather the eth balance of JBXBuybackDelegate is equal to 0 before the transaction, and greater than zero after the user attempts to buy a large amount of tokens (since no assumption can be made on the availability and price of the tokens can be made, I've used unrealistic numbers in this script for simplicity and clarity).

// 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';

/**
 * @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;
  
  //Q64.96 = 1 
  uint160 sqrtPriceX96 = 79228162514264337593543950336;

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


  function test_lockedEth() public {
    uint256 _weight = 1;
    // Reconfigure with a weight smaller than the quote, slippage included
    _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%
    );
    
    uint256 locked_eth = address(delegate).balance;
    console.log('Balance of delegate contract before swap:');
    console.log(locked_eth);
    assertEq(locked_eth, 0);
    
    // Pay the project
    jbEthPaymentTerminal.pay{value: 300000000000000000000000000}(
      1,
      300000000000000000000000000,
      address(0),
      address(123),
      /* _minReturnedTokens */
      0,
      /* _preferClaimedTokens */
      true,
      /* _memo */
      'Take my money!',
      /* _delegateMetadata */
      _metadata
    );

    uint256 c = jbx.balanceOf(address(pool));
    console.log("Balance of projectTokens left in V3 pool after the swap: ");
    console.log(c);
    locked_eth = address(delegate).balance;
    console.log('Balance of delegate contract after swap:');
    console.log(locked_eth);
    assertGt(locked_eth, 0);

  }

  


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

If you run this test with the -vvv option it will log the following content to the console:

[PASS] test_lockedEth() (gas: 26476658)
Logs:
  Bound Result 1
  Balance of delegate contract before swap:
  0
  Balance of projectTokens left in V3 pool after the swap: 
  1
  Balance of delegate contract after swap:
  289993998399739968996799679

Test result: ok. 1 passed; 0 failed; finished in 550.03ms

Recommended Mitigation Steps

Although the user can adjust the splippage, the contract should send back any leftover eth after it finishes all it's operations.

Assessed type

ETH-Transfer

[c4-pre-sort]

dmvt marked the issue as duplicate of #42

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as satisfactory