Juicebox Buyback Delegate - rbserver's results

Lines of code

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

Vulnerability details

Impact

Calling the following JBPayoutRedemptionPaymentTerminal3_1._pay function executes (_fundingCycle, _tokenCount, _delegateAllocations, _memo) = store.recordPaymentFrom(_payer, _bundledAmount, _projectId, baseWeightCurrency, _beneficiary, _memo, _metadata).

File: node_modules\@jbx-protocol\juice-contracts-v3\contracts\abstract\JBPayoutRedemptionPaymentTerminal3_1.sol
1444:   function _pay(
1445:     uint256 _amount,
1446:     address _payer,
1447:     uint256 _projectId,
1448:     address _beneficiary,
1449:     uint256 _minReturnedTokens,
1450:     bool _preferClaimedTokens,
1451:     string memory _memo,
1452:     bytes memory _metadata
1453:   ) internal returns (uint256 beneficiaryTokenCount) {
...
1469:       // Record the payment.
1470:       (_fundingCycle, _tokenCount, _delegateAllocations, _memo) = store.recordPaymentFrom(
1471:         _payer,
1472:         _bundledAmount,
1473:         _projectId,
1474:         baseWeightCurrency,
1475:         _beneficiary,
1476:         _memo,
1477:         _metadata
1478:       );
1479: 
1480:       // Mint the tokens if needed.
1481:       if (_tokenCount > 0)
1482:         // Set token count to be the number of tokens minted for the beneficiary instead of the total amount.
1483:         beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(
1484:           _projectId,
1485:           _tokenCount,
1486:           _beneficiary,
1487:           '',
1488:           _preferClaimedTokens,
1489:           true
1490:         );
1491: 
1492:       // The token count for the beneficiary must be greater than or equal to the minimum expected.
1493:       if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT();
1494: 
1495:       // If delegate allocations were specified by the data source, fulfill them.
1496:       if (_delegateAllocations.length != 0) {
...
1513:         // Get a reference to the number of delegates to allocate to.
1514:         uint256 _numDelegates = _delegateAllocations.length;
1515: 
1516:         for (uint256 _i; _i < _numDelegates; ) {
...
1523:           // Keep track of the msg.value to use in the delegate call
1524:           uint256 _payableValue;
1525: 
1526:           // If this terminal's token is ETH, send it in msg.value.
1527:           if (token == JBTokens.ETH) _payableValue = _delegateAllocation.amount;
...
1532:           _delegateAllocation.delegate.didPay{value: _payableValue}(_data);
...
1541:           unchecked {
1542:             ++_i;
1543:           }
1544:         }
1545:       }
1546:     }
...
1560:   }

Then, calling the following JBSingleTokenPaymentTerminalStore3_1.recordPaymentFrom function executes (_weight, memo, delegateAllocations) = IJBFundingCycleDataSource(fundingCycle.dataSource()).payParams(_data).

File: node_modules\@jbx-protocol\juice-contracts-v3\contracts\JBSingleTokenPaymentTerminalStore3_1.sol
323:   function recordPaymentFrom(
324:     address _payer,
325:     JBTokenAmount calldata _amount,
326:     uint256 _projectId,
327:     uint256 _baseWeightCurrency,
328:     address _beneficiary,
329:     string calldata _memo,
330:     bytes memory _metadata
331:   )
332:     external
333:     override
334:     nonReentrant
335:     returns (
336:       JBFundingCycle memory fundingCycle,
337:       uint256 tokenCount,
338:       JBPayDelegateAllocation[] memory delegateAllocations,
339:       string memory memo
340:     )
341:   {
...
351:     // The weight according to which new token supply is to be minted, as a fixed point number with 18 decimals.
352:     uint256 _weight;
353: 
354:     // If the funding cycle has configured a data source, use it to derive a weight and memo.
355:     if (fundingCycle.useDataSourceForPay() && fundingCycle.dataSource() != address(0)) {
356:       // Create the params that'll be sent to the data source.
357:       JBPayParamsData memory _data = JBPayParamsData(
358:         IJBSingleTokenPaymentTerminal(msg.sender),
359:         _payer,
360:         _amount,
361:         _projectId,
362:         fundingCycle.configuration,
363:         _beneficiary,
364:         fundingCycle.weight,
365:         fundingCycle.reservedRate(),
366:         _memo,
367:         _metadata
368:       );
369:       (_weight, memo, delegateAllocations) = IJBFundingCycleDataSource(fundingCycle.dataSource())
370:         .payParams(_data);
371:     }
372:     // Otherwise use the funding cycle's weight
373:     else {
...
376:     }
...
414:     // If there's no weight, token count must be 0 so there's nothing left to do.
415:     if (_weight == 0) return (fundingCycle, 0, delegateAllocations, memo);
...
428:   }

If _tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR) is true, calling the following JBXBuybackDelegate.payParams function would return a 0 weight. When _weight == 0 is true in the JBSingleTokenPaymentTerminalStore3_1.recordPaymentFrom function, it would return a 0 tokenCount. Back in the JBPayoutRedemptionPaymentTerminal3_1._pay function, when _tokenCount is 0, beneficiaryTokenCount would remain 0 that is its default value, and executing if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT() can revert when the _minReturnedTokens input is positive. Thus, in order ot make such transaction to go through, the user who calls the JBPayoutRedemptionPaymentTerminal3_1.pay function that further calls the JBPayoutRedemptionPaymentTerminal3_1._pay function must input _minReturnedTokens as 0 in this situation.

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

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

        // Unpack the quote from the pool, given by the frontend
        (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));

        // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
        if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
            // Pass the quote and reserve rate via a mutex
            mintedAmount = _tokenCount;
            reservedRate = _data.reservedRate;

            // Return this delegate as the one to use, and do not mint from the terminal
            delegateAllocations = new JBPayDelegateAllocation[](1);
            delegateAllocations[0] =
                JBPayDelegateAllocation({delegate: IJBPayDelegate(this), amount: _data.amount.value});

            return (0, _data.memo, delegateAllocations);
        }

        ...
    }

Furthermore, when the JBPayoutRedemptionPaymentTerminal3_1._pay function executes _delegateAllocation.delegate.didPay{value: _payableValue}(_data), the following JBXBuybackDelegate.didPay function can execute _mint(_data, _tokenCount) if the swap fails or _data.preferClaimedTokens is false. However, when calling the JBXBuybackDelegate._mint function below, the return value of the controller.mintTokensOf function call is not used to check against a value that is similar to _minReturnedTokens, which is unlike if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT(), where beneficiaryTokenCount is the return value of the IJBController(directory.controllerOf(_projectId)).mintTokensOf function call, in the JBPayoutRedemptionPaymentTerminal3_1._pay function. As a result, the amount of project token minted to the corresponding beneficiary is not checked against an expected minimum number of project tokens to be minted to such beneficiary. Because of such lack of slippage control, the actual amount of project token minted to the beneficiary can be lower than expected, such as when reservedRate is unexpectedly changed for the current funding cycle.

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

    function didPay(JBDidPayData calldata _data) external payable override {
        ...

        // Retrieve the number of token created if minting and reset the mutex (not exposed in JBDidPayData)
        uint256 _tokenCount = mintedAmount;
        mintedAmount = 1;

        // Retrieve the fc reserved rate and reset the mutex
        uint256 _reservedRate = reservedRate;
        reservedRate = 1;

        // The minimum amount of token received if swapping
        (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
        uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);

        // Pick the appropriate pathway (swap vs mint), use mint if non-claimed prefered
        if (_data.preferClaimedTokens) {
            // Try swapping
            uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);

            // If swap failed, mint instead, with the original weight + add to balance the token in
            if (_amountReceived == 0) _mint(_data, _tokenCount);
        } else {
            _mint(_data, _tokenCount);
        }
    }

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

    function _mint(JBDidPayData calldata _data, uint256 _amount) internal {
        IJBController controller = IJBController(jbxTerminal.directory().controllerOf(_data.projectId));

        // Mint to the beneficiary with the fc reserve rate
        controller.mintTokensOf({
            _projectId: _data.projectId,
            _tokenCount: _amount,
            _beneficiary: _data.beneficiary,
            _memo: _data.memo,
            _preferClaimedTokens: _data.preferClaimedTokens,
            _useReservedRate: true
        });

        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the JBPayoutRedemptionPaymentTerminal3_1.pay function, which further calls the JBPayoutRedemptionPaymentTerminal3_1._pay function, to pay some ETH.
2. Because _tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR) in the JBXBuybackDelegate.payParams function would be true for her transaction, the swap pathway could be used.
3. In order to make her transaction go through, Alice's transaction uses 0 as the _minReturnedTokens input for the JBPayoutRedemptionPaymentTerminal3_1.pay function call.
4. Yet, the swap fails for some reason so the JBXBuybackDelegate._mint function is called.
5. Since the _minReturnedTokens input is 0 when calling the JBPayoutRedemptionPaymentTerminal3_1.pay function and the amount of project token minted to her beneficiary by the JBXBuybackDelegate._mint function is not checked against an expected minimum number of project tokens to be minted to such beneficiary, an unexpected change of reservedRate for the current funding cycle can cause the actual amount of project token minted to her beneficiary to be lower than expected.

Tools Used

VSCode

Recommended Mitigation Steps

The JBPayoutRedemptionPaymentTerminal3_1._pay function can be updated to execute if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT() only when _tokenCount > 0 is true and pass _minReturnedTokens to the JBXBuybackDelegate.didPay function. Then, the JBXBuybackDelegate.didPay and JBXBuybackDelegate._mint functions can be updated to be able to receive _minReturnedTokens. Moreover, the JBXBuybackDelegate._mint function can be updated to revert if the return value of the controller.mintTokensOf function call is less than _minReturnedTokens.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L183-L209 https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L258-L326 https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L216-L233

Vulnerability details

Impact

When calling the following JBPayoutRedemptionPaymentTerminal3_1._pay function, if _delegateAllocations.length != 0 is true, _delegateAllocation.delegate.didPay{value: _payableValue}(_data) can be executed, which sends _delegateAllocation.amount as msg.value to the JBXBuybackDelegate.didPay function below when the terminal's token is ETH.

File: node_modules\@jbx-protocol\juice-contracts-v3\contracts\abstract\JBPayoutRedemptionPaymentTerminal3_1.sol
1444:   function _pay(
1445:     uint256 _amount,
1446:     address _payer,
1447:     uint256 _projectId,
1448:     address _beneficiary,
1449:     uint256 _minReturnedTokens,
1450:     bool _preferClaimedTokens,
1451:     string memory _memo,
1452:     bytes memory _metadata
1453:   ) internal returns (uint256 beneficiaryTokenCount) {
...
1495:       // If delegate allocations were specified by the data source, fulfill them.
1496:       if (_delegateAllocations.length != 0) {
...
1513:         // Get a reference to the number of delegates to allocate to.
1514:         uint256 _numDelegates = _delegateAllocations.length;
1515: 
1516:         for (uint256 _i; _i < _numDelegates; ) {
1517:           // Get a reference to the delegate being iterated on.
1518:           JBPayDelegateAllocation memory _delegateAllocation = _delegateAllocations[_i];
1519: 
1520:           // Trigger any inherited pre-transfer logic.
1521:           _beforeTransferTo(address(_delegateAllocation.delegate), _delegateAllocation.amount);
1522: 
1523:           // Keep track of the msg.value to use in the delegate call
1524:           uint256 _payableValue;
1525: 
1526:           // If this terminal's token is ETH, send it in msg.value.
1527:           if (token == JBTokens.ETH) _payableValue = _delegateAllocation.amount;
...
1532:           _delegateAllocation.delegate.didPay{value: _payableValue}(_data);
...
1541:           unchecked {
1542:             ++_i;
1543:           }
1544:         }
1545:       }
1546:     }
...
1560:   }

Then, calling the following JBXBuybackDelegate.didPay function calls the JBXBuybackDelegate._swap function, which further calls the JBXBuybackDelegate.uniswapV3SwapCallback function below when calling pool.swap(...).

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

    function didPay(JBDidPayData calldata _data) external payable override {
        ...
        // Pick the appropriate pathway (swap vs mint), use mint if non-claimed prefered
        if (_data.preferClaimedTokens) {
            // Try swapping
            uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);
            ...
        } else {
            ...
        }
    }

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

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        // Pass the token and min amount to receive as extra data
        try pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
            data: abi.encode(_minimumReceivedFromSwap)
        }) returns (int256 amount0, int256 amount1) {
            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));
        } catch {
            // implies _amountReceived = 0 -> will later mint when back in didPay
            return _amountReceived;
        }
        ...
    }

In the following the JBXBuybackDelegate.uniswapV3SwapCallback function, weth.deposit{value: _amountToSend}() is executed, where _amountToSend is uint256(_projectTokenIsZero ? amount1Delta : amount0Delta). When the liquidity of the corresponding pool is not sufficient enough, it is possible that partial fill occurs; in this case, _amountToSend, which is amount1Delta or amount0Delta provided by the UniswapV3Pool.swap function, would be smaller than int256(_data.amount.value) that is used as amountSpecified for calling pool.swap(...) in the JBXBuybackDelegate._swap function. Since _data.amount.value is same as _delegateAllocation.amount that is used as msg.value for calling the JBXBuybackDelegate.didPay function in the JBPayoutRedemptionPaymentTerminal3_1._pay function, the difference between _delegateAllocation.amount and _amountToSend is not sent to the corresponding pool and is locked in the JBXBuybackDelegate contract; hence, the user who pays _delegateAllocation.amount ETH loses such difference. As shown in https://info.uniswap.org/#/pools/0x48598ff1cee7b4d31f8f9050c2bbae98e17e6b17 and https://info.uniswap.org/#/pools/0x632881d9f6231bee100cf7d060cc27d86b2a5cdb, the TVLs for the JBX / ETH 1% and 0.3% pools are small so these pools' liquidity can be insufficient and the described situation can happen.

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

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external override {
        // Check if this is really a callback
        if (msg.sender != address(pool)) revert JuiceBuyback_Unauthorized();

        // Unpack the data
        (uint256 _minimumAmountReceived) = abi.decode(data, (uint256));

        // Assign 0 and 1 accordingly
        uint256 _amountReceived = uint256(-(_projectTokenIsZero ? amount0Delta : amount1Delta));
        uint256 _amountToSend = uint256(_projectTokenIsZero ? amount1Delta : amount0Delta);

        // Revert if slippage is too high
        if (_amountReceived < _minimumAmountReceived) revert JuiceBuyback_MaximumSlippage();

        // Wrap and transfer the weth to the pool
        weth.deposit{value: _amountToSend}();
        weth.transfer(address(pool), _amountToSend);
    }

https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L596-L788

    function swap(
        address recipient,
        bool zeroForOne,
        int256 amountSpecified,
        uint160 sqrtPriceLimitX96,
        bytes calldata data
    ) external override noDelegateCall returns (int256 amount0, int256 amount1) {
        ...

        (amount0, amount1) = zeroForOne == exactInput
            ? (amountSpecified - state.amountSpecifiedRemaining, state.amountCalculated)
            : (state.amountCalculated, amountSpecified - state.amountSpecifiedRemaining);

        // do the transfers and collect payment
        if (zeroForOne) {
            if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1));

            uint256 balance0Before = balance0();
            IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amount0, amount1, data);
            require(balance0Before.add(uint256(amount0)) <= balance0(), 'IIA');
        } else {
            if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0));

            uint256 balance1Before = balance1();
            IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amount0, amount1, data);
            require(balance1Before.add(uint256(amount1)) <= balance1(), 'IIA');
        }

        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the JBPayoutRedemptionPaymentTerminal3_1.pay function, which further calls the JBPayoutRedemptionPaymentTerminal3_1._pay function, to pay 0.5e18 wei ETH.
2. When the JBXBuybackDelegate.uniswapV3SwapCallback function is eventually called, _amountToSend can be 0.3e18 because the corresponding pool's liquidity is not sufficient and partial fill occurs.
3. Due to the partial fill, 0.5e18 - 0.3e18 = 0.2e18 wei ETH are not used for the swap. Since such 0.2e18 wei ETH are now locked in the JBXBuybackDelegate contract, Alice loses them.

Tools Used

VSCode

Recommended Mitigation Steps

The JBXBuybackDelegate.didPay function can be updated to refund the extra ETH payment amount that is not used for swapping back to the user who calls the JBPayoutRedemptionPaymentTerminal3_1.pay function or send such extra ETH amount to the corresponding beneficiary.

Assessed type

Other

QA REPORT

[01] projectToken.transfer(_data.projectId, _data.beneficiary, _nonReservedToken) CAN BE EXECUTED INSTEAD OF projectToken.transfer(_data.beneficiary, _nonReservedToken) IN JBXBuybackDelegate._swap FUNCTION

When calling the following JBXBuybackDelegate._swap function, if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken) is executed. In the JBXBuybackDelegate contract, if projectToken is set to a JBToken contract, which has projectId that is not _data.projectId used for calling the JBXBuybackDelegate._swap function, and pool is set to a pool for swapping such projectToken due to an intentional or unintentional misconfiguration, then calling the JBXBuybackDelegate._swap function can transfer _nonReservedToken amount of such projectToken to _data.beneficiary. No matter if such misconfiguration is intentional and malicious or unintentional and accidental, the beneficiary specified by the user can receive projectToken that is not related to the corresponding project and can be worthless after such user pays the project. To safeguard against such misconfiguration, please consider executing projectToken.transfer(_data.projectId, _data.beneficiary, _nonReservedToken) instead of projectToken.transfer(_data.beneficiary, _nonReservedToken) in the JBXBuybackDelegate._swap function because the JBToken.transfer(uint256 _projectId, address _to, uint256 _amount) function would execute if (projectId != 0 && _projectId != projectId) revert BAD_PROJECT().

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

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        ...

        // Send the non-reserved token to the beneficiary (if any / reserved rate is not max)
        if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken);

        ...
    }

File: node_modules\@jbx-protocol\juice-contracts-v3\contracts\JBToken.sol
182:   function transfer(
183:     uint256 _projectId,
184:     address _to,
185:     uint256 _amount
186:   ) external override {
187:     // Can't transfer for a wrong project.
188:     if (projectId != 0 && _projectId != projectId) revert BAD_PROJECT();
189: 
190:     transfer(_to, _amount);
191:   }

[02] JBXBuybackDelegate CONTRACT'S CONSTRUCTOR CAN BE UPDATED TO CHECK IF _pool IS FOR _projectToken

Calling the following constructor in the JBXBuybackDelegate contract would set projectToken and pool. Yet, if not being careful enough, it is possible that pool can be set to a pool that is not for projectToken. If this occurs, calling the JBXBuybackDelegate._swap function can transfer amount of a token that is not projectToken to the JBXBuybackDelegate contract but revert when executing projectToken.transfer(_data.beneficiary, _nonReservedToken) since the JBXBuybackDelegate contract does not own any projectToken. To prevent this unexpected behavior from happening, please consider checking if _pool is for _projectToken in the JBXBuybackDelegate contract's constructor and reverting such constructor if _pool is not for _projectToken.

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

    constructor(
        IERC20 _projectToken,
        IWETH9 _weth,
        IUniswapV3Pool _pool,
        IJBPayoutRedemptionPaymentTerminal3_1 _jbxTerminal
    ) {
        projectToken = _projectToken;
        pool = _pool;
        ...
    }

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

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        // Pass the token and min amount to receive as extra data
        try pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
            data: abi.encode(_minimumReceivedFromSwap)
        }) returns (int256 amount0, int256 amount1) {
            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));
        } catch {
            // implies _amountReceived = 0 -> will later mint when back in didPay
            return _amountReceived;
        }

        // The amount to send to the beneficiary
        uint256 _nonReservedToken = PRBMath.mulDiv(
            _amountReceived, JBConstants.MAX_RESERVED_RATE - _reservedRate, JBConstants.MAX_RESERVED_RATE
        );

        ...

        // Send the non-reserved token to the beneficiary (if any / reserved rate is not max)
        if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken);

        ...
    }

[03] THERE IS NO SWAP DEADLINE FOR CALLING JBXBuybackDelegate._swap FUNCTION

Calling the following JBXBuybackDelegate._swap function makes the pool.swap function call, which does not enforce a deadline for the swap. It is possible that the transaction for the swap remains pending in the mempool for a long time, such as due to the accompanying transaction fee being too low. Later, when a miner finally includes such transaction, the amount of the project token being swapped out can be much less than that if such transaction could be executed when it was just sent, such as because of the MEV bots' operations for attempting to manipulate the pool's price. Although the JBXBuybackDelegate.uniswapV3SwapCallback function does execute if (_amountReceived < _minimumAmountReceived) revert JuiceBuyback_MaximumSlippage(), it can be less optimal for the user without enforcing the swap deadline since the amount of the project token being swapped out at the time when the transaction is finally executed can be equal to or just higher than _minimumAmountReceived while such token amount being swapped out could be much higher than _minimumAmountReceived if such transaction could be executed when it was just sent. Hence, to optimize the swap functionality and prevent the MEV bots and other malicious actors from taking advantages of the transaction that calls the JBXBuybackDelegate._swap function, _data.metadata for the JBXBuybackDelegate.didPay function can be updated to allow the user to encode a specified deadline for the swap, and the JBXBuybackDelegate.uniswapV3SwapCallback function can be updated to revert if block.timestamp has passed such swap deadline.

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

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        // Pass the token and min amount to receive as extra data
        try pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
            data: abi.encode(_minimumReceivedFromSwap)
        }) returns (int256 amount0, int256 amount1) {
            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));
        } catch {
            // implies _amountReceived = 0 -> will later mint when back in didPay
            return _amountReceived;
        }

        ...
    }

[04] pool IS NOT UPDATABLE IN JBXBuybackDelegate CONTRACT

As shown below, pool is an immutable, which is assigned after calling the JBXBuybackDelegate contract's constructor. It is possible that the liquidity of the assigned pool becomes much smaller in the future. However, when this happens, pool cannot be updated to a relevant pool that has a higher liquidity. To avoid continuing using a pool that has a small liquidity, please consider changing pool to not be an immutable and adding a function, which should be only callable by the corresponding project's admin, for updating pool.

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

    IUniswapV3Pool public immutable pool;

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

    constructor(
        IERC20 _projectToken,
        IWETH9 _weth,
        IUniswapV3Pool _pool,
        IJBPayoutRedemptionPaymentTerminal3_1 _jbxTerminal
    ) {
        ...
        pool = _pool;
        ...
    }

[05] MISSING address(0) CHECKS FOR CRITICAL CONSTRUCTOR INPUTS

To prevent unintended behaviors, critical constructor inputs should be checked against address(0). address(0) checks are missing for _projectToken, _weth, _pool, and _jbxTerminal in the following constructor. Please consider checking them.

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

    constructor(
        IERC20 _projectToken,
        IWETH9 _weth,
        IUniswapV3Pool _pool,
        IJBPayoutRedemptionPaymentTerminal3_1 _jbxTerminal
    ) {
        projectToken = _projectToken;
        pool = _pool;
        jbxTerminal = _jbxTerminal;
        _projectTokenIsZero = address(_projectToken) < address(_weth);
        weth = _weth;
    }

[06] _nonReservedToken CAN BE USED, INSTEAD OF _nonReservedTokenInContract, AS _tokenCount FOR CALLING controller.burnTokensOf IN JBXBuybackDelegate._swap FUNCTION

In the following JBXBuybackDelegate._swap function, uint256 _reservedToken = _amountReceived - _nonReservedToken is executed before executing uint256 _nonReservedTokenInContract = _amountReceived - _reservedToken. Since _amountReceived, _nonReservedToken, and _reservedToken are not changed between these two operations, _nonReservedTokenInContract is basically same as _nonReservedToken. Hence, to make the code more efficient, please consider using _nonReservedToken, instead of calculating and using _nonReservedTokenInContract, as _tokenCount for calling controller.burnTokensOf.

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

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        ...

        // The amount to add to the reserved token
        uint256 _reservedToken = _amountReceived - _nonReservedToken;

        ...

        // If there are reserved token, add them to the reserve
        if (_reservedToken != 0) {
            ...
            uint256 _nonReservedTokenInContract = _amountReceived - _reservedToken;

            if (_nonReservedTokenInContract != 0) {
                controller.burnTokensOf({
                    _holder: address(this),
                    _projectId: _data.projectId,
                    _tokenCount: _nonReservedTokenInContract,
                    _memo: "",
                    _preferClaimedTokens: false
                });
            }
        }

        ...
    }

[07] IJBFundingCycleBallot INTERFACE IS NOT USED IN JBXBuybackDelegate CONTRACT

The IJBFundingCycleBallot interface is not used in the JBXBuybackDelegate contract, please consider removing the import statement for it for better code efficiency.

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

import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleBallot.sol";

[08] SLIPPAGE_DENOMINATOR CAN BE ADDED TO AND IMPORTED FROM JBConstants LIBRARY

For a better code organization, the following SLIPPAGE_DENOMINATOR constant can be added to and imported from the JBConstants library.

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

    uint256 private constant SLIPPAGE_DENOMINATOR = 10000;

[09] UNDERSCORE CAN BE ADDED FOR SLIPPAGE_DENOMINATOR

It is a common practice to separate each 3 digits in a number by an underscore to improve code readability. The following SLIPPAGE_DENOMINATOR constant does not use any underscores; please consider adding an underscore for it.

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

    uint256 private constant SLIPPAGE_DENOMINATOR = 10000;

[10] REDUNDANT NAMED RETURN FOR delegateAllocations

( Please note that the following instance is not found in https://gist.github.com/itsmetechjay/2efc963de59bcad62e69de48171d10ca#n05-adding-a-return-statement-when-the-function-defines-a-named-return-variable-is-redundant. )

When a function has unused named returns and used return statement, these named returns become redundant. To improve maintainability, the named return for delegateAllocations can be removed while a temporary variable for delegateAllocations can be used in the corresponding return statement in the following JBXBuybackDelegate.payParams function.

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

    function payParams(JBPayParamsData calldata _data)
        external
        override
        returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
    {
        ...

        // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
        if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
            ...

            // Return this delegate as the one to use, and do not mint from the terminal
            delegateAllocations = new JBPayDelegateAllocation[](1);
            delegateAllocations[0] =
                JBPayDelegateAllocation({delegate: IJBPayDelegate(this), amount: _data.amount.value});

            return (0, _data.memo, delegateAllocations);
        }

        // If minting, do not use this as delegate
        return (_data.weight, _data.memo, new JBPayDelegateAllocation[](0));
    }

[11] control IS MISTYPED AS controle

( Please note that the following instance is not found in https://gist.github.com/itsmetechjay/2efc963de59bcad62e69de48171d10ca#n13-typos. )

In the following comment, control is mistyped as controle. Please change controle to control.

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

     * @dev    Slippage controle is achieved here

[12] SOLIDITY VERSION 0.8.20 CAN BE USED

Using the more updated version of Solidity can add new features and enhance security. As described in https://github.com/ethereum/solidity/releases, Version 0.8.20 is the latest version of Solidity, which includes support for Shanghai. To be more secured and more future-proofed, please consider using Version 0.8.20 for the JBXBuybackDelegate contract.

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

pragma solidity ^0.8.16;