Lines of code

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

Vulnerability details

Impact

During the normal execution flow from Juicebox protocol pay call (JBPayoutRedemptionPaymentTerminal3_1::pay), JBXBuybackDelegate::payParams will eventually be called. It sets minting tokens amount and the reserver token rate then JBXBuybackDelegate::didPay function will be called that uses the cached data to transfer tokens accordingly. Because of a missing access control check, payParams can be called by anybody (didPay has a check that only the terminal may call it) and as such, between the first payParams call and the didPay call, any compromised external call, however down the line in the terminal, can recall payParams to manipulate the controller minted amounts leading to a protocol token inflation attack.

Vulnerability details

Normal execution flow

In the context of an execution flow behind Juicebox protocol: when operating with a funding cycle that has a treasury data source that extends the treasury's pay functionality, there is call to the IJBFundingCycleDataSource function payParams followed by a function call to the corresponding delegate contract (IJBPayDelegate) didPay pay function.

JBXBuybackDelegate extends both interfaces and as such, implements both logics.

In payParams the token amount to be minted and number of tokens to be reserved are saved in a state variable.

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

            mintedAmount = _tokenCount;
            reservedRate = _data.reservedRate;

Subsequently they are used further down the execution flow in didPay as tokens to mint count (to the indicated beneficiary address) and how much tokens to reserve.

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

        uint256 _tokenCount = mintedAmount;
        mintedAmount = 1;

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

        // ... 
        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);

Minting is then done further down the execution flow, directly on the controller of the indicated project.

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

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

It is important to observe here that the passed ETH to the function didPay is passed as it is

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

        jbxTerminal.addToBalanceOf{value: _data.amount.value}(
            _data.projectId, _data.amount.value, JBTokens.ETH, "", new bytes(0)
        );

and it is not corelate with the amount to mint tokens (_amount) in the context of the _mint function. As such, the only checks are done in payParams and in the terminal logic itself.

Overview of normal execution flow is as follows:

terminal.pay(..)
    -> terminalStore.recordPaymentFrom(..)
        -> JBXBuybackDelegate.payParams()
            - normal valid inputs are sent as to continue with the delegation call
    - returns the received delegate address (JBXBuybackDelegate)
-> JBXBuybackDelegate.didBuy(..)
    -> JBXBuybackDelegate._mint
        -> controller.mintTokensOf(..)
            - mints the correct number of tokens to the beneficiary address

The minting token count (that is cached between calls) is determined in JBXBuybackDelegate::payParams by this formula: https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L149-L150

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

In a normal execution flow
- weight (and reserve rate) is validated and provided by the current funding cycle
```
    fundingCycle.weight,
    fundingCycle.reservedRate(),
```
- _amount is validated when an EOA or external contract calls pay function. For the JBXBuybackDelegate contract, as it supports only ETH, it will map the amount 1 to 1
```
// If this terminal's token is ETH, override _amount with msg.value.
else _amount = msg.value;
```

Hijacked flow

A malicious actor with a hook in any intermediary external call can craft a simple pay request that reaches JBXBuybackDelegate::payParams with validated data accordingly by the terminal, then, as he has a callback, recall the payParams with manipulated values so that to mint infinite tokens from the project controller. Then return execution to the normal flow.

In the terminal (JBPayoutRedemptionPaymentTerminal3_1) between the JBXBuybackDelegate::payParams and JBXBuybackDelegate::didBuy call there are exactly 2 external calls (to project contracts):

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

      : prices.priceFor(_amount.currency, _baseWeightCurrency, _decimals);

2) https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1483

        beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(

Going into depth on the second call, IJBController, JBController3_1::mintTokensOf has exactly 8 unique external calls to 4 different project contracts:

JBFundingCycleStore
JBProjects
JBDirectory
JBFundingCycle(JBFundingCycleMetadataResolver)
JBTokenStore

Each of which also touches imports and so on.

For transparency, currently there is no know direct external call out of the Juicebox project (at least not known to the author of this issue).

The issue still lies that any unaccounted external call or change, improvement, addition to Juicebox terminal logic or anywhere down the execution line, that brings with it any external call, also brings this immediate threat since any future implementation/extension has a high probability of checking reentrance/external call logic only withing itself, not the whole of the project, subsequently the issue that payParams has a high probability of being missed.

It is also little to no effort for protocol developers to ensure this does not happen, and increase user safety, by simply checking that the caller to JBXBuybackDelegate::payParams is the terminal, exactly how it is already done for the JBXBuybackDelegate::didPay but for some reason was omitted on payParams.

As such, a medium severity is likely appropriate.

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

        // Access control as minting is authorized to this delegate
        if (msg.sender != address(jbxTerminal)) revert JuiceBuyback_Unauthorized();

Proof of Concept

Lack of access control

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a check in JBXBuybackDelegate::payParams that caller is the terminal, similar to how this is checked in JBXBuybackDelegate::didPay. Example:

        // Access control as minting is authorized to this delegate
        if (msg.sender != address(jbxTerminal)) revert JuiceBuyback_Unauthorized();

Assessed type

Access Control

code-423n4 / 2023-05-juicebox-findings

Lack of access control on `JBXBuybackDelegate::payParams` allows `JBPayoutRedemptionPaymentTerminal3_1::pay` calls to be manipulated which could lead to infinite token minting via the controller #121

Lines of code

Vulnerability details

Impact

Vulnerability details

Normal execution flow

Hijacked flow

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type