User lacks slippage protection if the swap fails

[code423n4]

Lines of code

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

Vulnerability details

Impact

There exists an inconsistency on the JBPayoutRedemptionPaymentTerminal3_1.sol contract when managing slippage of minted project tokens when the BuybackDelegate.sol contract is also used.

The pay() function at the terminal contract has a parameter named _minReturnedTokens where a user can specify the minimum project tokens to get minted, if not, it will revert. This slippage is important since in Ethereum you do not have guarantees when your tx will get confirmed.

The problem is that in order to take advantage of the feature of BuybackDelegate.sol the pay() function must be called with _minReturnedTokens as zero, so the user now does not have slippage protection on the potentially minted tokens, and the BuybackDelegate.sol does have slippage protection for the swap, but it does not have slippage protection if the swap fails and the user gets minted tokens instead.

Proof of Concept

Imagine a scenario where a user wants to contribute 1 ETH to the JX project. The front-end (or the user if he does not want to use a front end) reads the quote of the current Uniswap V3 pool and sees that it has a better quote (10,500 tokens for example) than the current weight (10,000 tokens for example). So, it crafts a transaction for the pay() function, it puts the quote and the swap slippage (minimum 10,001 tokens) on the metadata and sets _minReturnedTokens to zero.

It happens that the user tx does not get executed right away because of network congestion, or because the tx was held maliciously. During this time a lot can happen for example that weight has decreased (from 10,000 to 9,000 for example) because of a re-configuration or just the standard decrease after a funding cycle, also the quote of the uniswap pool can change (from 10,500 to 9,500 for example), regarding the uniswap pool more things can happen, for example liquidity decrement is also a possibility. When the user tx gets finally executed, the swap will fail because the slippage was set to get at least 10,001 tokens no 9,500. So, the user will get 9,000 minted tokens.

The problem is that the user never got the chance to specify a slippage of minted tokens in case the swap fails due to the non-determinism of transaction execution on Ethereum. Maybe he would not have contributed for only 9,000 tokens.

I wrote a PoC to show the behaviour described, it is important to note that on the test the user "pay()" transaction was sent after the changes on the weight and to the univ3 pool, because the tests are sequential, but in reality what can happen is that the tx was sent before but not executed if not after the changes described.

1.- Under the DelegateE2E.t.sol test add the following state variables:

uint256 liquidityNFT;
uint128 initialLiquidity;
address PM = 0xC36442b4a4522E871399CD717aBDD847Ab11FE88;

2.- On the setUp() function write the following when minting the liquidity:

(liquidityNFT, initialLiquidity, , ) = INonfungiblePositionManager(POSITION_MANAGER).mint(params);

3.- Copy and paste the following test:

function test_noSlippageProtection() public {
    uint256 _weight = (amountOutForOneEth * 9) / 10;
    _reconfigure(1, address(delegate), _weight, 0);

    // The metadata is made using the quote at the current block
    // Quote is better than weight.
    bytes memory _metadata = abi.encode(
        bytes32(0),
        bytes32(0),
        amountOutForOneEth, //quote
        500 //5% slippage 
    );

    // User cannot set a minReturnedTokens greater than zero if he wants to take advantage of having
    // a better quote than a weight.
    vm.expectRevert();    
    jbEthPaymentTerminal.pay{value: 1 ether}(
      1,
      1 ether,
      address(0),
      address(123),
      /* _minReturnedTokens */
      1,
      /* _preferClaimedTokens */
      true,
      /* _memo */
      'Take my money!',
      /* _delegateMetadata */
      _metadata
    );

    INonfungiblePositionManager.DecreaseLiquidityParams memory paramsDecrease =
      INonfungiblePositionManager.DecreaseLiquidityParams({
        tokenId: liquidityNFT,
        liquidity: initialLiquidity,
        amount0Min: 0,
        amount1Min: 0,
        deadline: block.timestamp
    }); 

    // Liquidity decrement
    vm.startPrank(address(123));
    INonfungiblePositionManager(PM).decreaseLiquidity(paramsDecrease);
    vm.stopPrank();

    // Weight decreases
    uint256 _weight2 = (amountOutForOneEth * 8) / 10;
    _reconfigure(1, address(delegate), _weight2, 0);

    /*
      It is important to understand that this transaction was sent previous the change of the weight
      and the decrease of the liquidity. What could have happen is a network congestion gas spike or
      the tx of the user was hold maliciously. 
    */
    jbEthPaymentTerminal.pay{value: 1 ether}(
      1,
      1 ether,
      address(0),
      address(123),
      /* _minReturnedTokens */
      0,
      /* _preferClaimedTokens */
      true,
      /* _memo */
      'Take my money!',
      /* _delegateMetadata */
      _metadata
    );

    // The swap failed so the user got minted _weight2.
    assertEq(jbx.balanceOf(address(123)), _weight2);
  }

4.- run:

forge test --match-contract TestIntegrationJBXBuybackDelegate --match-test test_noSlippageProtection

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users set a minimum amount of tokens to get minted if the swap fails.

Assessed type

Other

[c4-pre-sort]

dmvt marked the issue as duplicate of #7

[c4-pre-sort]

dmvt marked the issue as not a duplicate

[c4-pre-sort]

dmvt marked the issue as low quality report

[dmvt]

Slippage protection exists. User passes the value of quote in params.

[c4-judge]

dmvt marked the issue as unsatisfactory:
Invalid

[0xRobocop]

I think the issue was miss-judged

I do not agree with the label of low quality report since it explains the issue of having to set _minReturnedTokens to zero and how this leaves the user unprotected on the "mint" path, and provides a proof of concept for it.

@dmvt you mentioned that a slippage exists as a param, but the issue clearly describes that _minReturnedTokens needs to be set to zero, which leaves the user without protection if the "vainilla path" is taken (this is decided at running time) or when it defaults to the vainilla path because the swap failed.

This issue is a duplicate of #232

Quoting issue 232

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

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked the issue as duplicate of #232

[dmvt]

I think the issue was miss-judged

I do not agree with the label of low quality report since it explains the issue of having to set _minReturnedTokens to zero and how this leaves the user unprotected on the "mint" path, and provides a proof of concept for it.

@dmvt you mentioned that a slippage exists as a param, but the issue clearly describes that _minReturnedTokens needs to be set to zero, which leaves the user without protection if the "vainilla path" is taken (this is decided at running time) or when it defaults to the vainilla path because the swap failed.

This issue is a duplicate of #232

Quoting issue 232

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

You're correct. Good catch.