Juicebox Buyback Delegate - adriro's results

Lines of code

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

Vulnerability details

Delegate architecture forces users to set zero slippage

The design of the delegate forces users to set a zero value for the _minReturnedTokens parameter when calling pay() in the terminal.

Technical details

In order to implement the swap functionality, the JBXBuybackDelegate needs to signal the terminal to not mint any tokens when the swap path is token. This done by setting the weight variable to zero:

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

144:     function payParams(JBPayParamsData calldata _data)
145:         external
146:         override
147:         returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
148:     {
149:         // Find the total number of tokens to mint, as a fixed point number with 18 decimals
150:         uint256 _tokenCount = PRBMath.mulDiv(_data.amount.value, _data.weight, 10 ** 18);
151: 
152:         // Unpack the quote from the pool, given by the frontend
153:         (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
154: 
155:         // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
156:         if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
157:             // Pass the quote and reserve rate via a mutex
158:             mintedAmount = _tokenCount;
159:             reservedRate = _data.reservedRate;
160: 
161:             // Return this delegate as the one to use, and do not mint from the terminal
162:             delegateAllocations = new JBPayDelegateAllocation[](1);
163:             delegateAllocations[0] =
164:                 JBPayDelegateAllocation({delegate: IJBPayDelegate(this), amount: _data.amount.value});
165: 
166:             return (0, _data.memo, delegateAllocations);
167:         }
168: 
169:         // If minting, do not use this as delegate
170:         return (_data.weight, _data.memo, new JBPayDelegateAllocation[](0));
171:     }

This can be seen in line 166 in the previous snippet of code, where the function returns zero as the weight return value when going the swap path. This weight is then used to calculate the tokenCount in the JBSingleTokenPaymentTerminalStore3_1 contract:

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

...
tokenCount = PRBMath.mulDiv(_amount.value, _weight, _weightRatio);
...

This tokenCount variable is finally returned to the JBPayoutRedemptionPaymentTerminal3_1 contract, which uses it to calculate the amount of tokens to mint and validate the tokens assigned to the beneficiary are above the minimum:

https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/abstract/JBPayoutRedemptionPaymentTerminal3_1.sol#L1470-L1493

...

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

// Mint the tokens if needed.
if (_tokenCount > 0)
  // Set token count to be the number of tokens minted for the beneficiary instead of the total amount.
  beneficiaryTokenCount = IJBController(directory.controllerOf(_projectId)).mintTokensOf(
    _projectId,
    _tokenCount,
    _beneficiary,
    '',
    _preferClaimedTokens,
    true
  );

// The token count for the beneficiary must be greater than or equal to the minimum expected.
if (beneficiaryTokenCount < _minReturnedTokens) revert INADEQUATE_TOKEN_COUNT();

...

This means that if the swap path is chosen, weight is going to be zero, which sets _tokenCount to zero, and implies that beneficiaryTokenCount is going to be zero as well. Setting _minReturnedTokens to any value different than zero will make the transaction revert.

However, the "buyback" path is not the only alternative. The delegate could also take the "vanilla" path in which it simply bypasses the delegate and tokens are minted in the terminal (line 170 in payParams()). This creates a conflict between both paths that forces the user to set _minReturnedTokens to zero, because the taken path is resolved at transaction time. Setting _minReturnedTokens to any value different than zero will make the "buyback" path revert, while setting _minReturnedTokens to zero nullifies the slippage check when the "vanilla" path is taken.

Impact

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

Recommendation

It is difficult to give a recommendation that doesn't involve significant changes to the code, as the current design relies on returning a zero weight when choosing the "buyback" path, which forces the condition in _minReturnedTokens in order to prevent the transaction revert. Nevertheless, the situation should be revised, as setting a zero value for _minReturnedTokens can have a significant impact on users engaging with the protocol.

Assessed type

Other

Lines of code

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

Vulnerability details

Delegate doesn't verify payed ETH value matches amount in parameter

The JBXBuybackDelegate delegate fails to check that the sent ETH amount matches the value passed in the amount parameter.

Impact

The payable didPay() function present in the delegate is called by forwarding ETH as callvalue and an amount of tokens that is specified in the JBDidPayData struct present in the parameter of the function (_data.amount.value).

However, the function doesn't verify that the value of ETH sent equals the amount of tokens present in the argument of the function. ETH could be lost if more value is sent than the specified amount in the parameter of the function, as the implementation then uses this value to operate.

Recommendation

Make sure the forwarded ETH value equals the amount passed in the JBDidPayData struct parameter.

function didPay(JBDidPayData calldata _data) external payable override {
    // Access control as minting is authorized to this delegate
    if (msg.sender != address(jbxTerminal)) revert JuiceBuyback_Unauthorized();
    
    require(msg.value == _data.amount.value);
    
    ...

Assessed type

Other

Lines of code

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

Vulnerability details

Swaps in Uniswap V3 may be partial

Uniswap V3 pools may execute a swap partially, in which case it may leave an unhandled amount of WETH in the JBXBuybackDelegate contract.

Impact

Swaps in Uniswap V3 can eventually be executed partially, if liquidity is not enough or if slippage tolerance is hit. This means that not all of the input token amount is consumed during the swap.

In the context of JBXBuybackDelegate, we can see this being a potential issue in the uniswapV3SwapCallback() callback:

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

216:     function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external override {
217:         // Check if this is really a callback
218:         if (msg.sender != address(pool)) revert JuiceBuyback_Unauthorized();
219: 
220:         // Unpack the data
221:         (uint256 _minimumAmountReceived) = abi.decode(data, (uint256));
222: 
223:         // Assign 0 and 1 accordingly
224:         uint256 _amountReceived = uint256(-(_projectTokenIsZero ? amount0Delta : amount1Delta));
225:         uint256 _amountToSend = uint256(_projectTokenIsZero ? amount1Delta : amount0Delta);
226: 
227:         // Revert if slippage is too high
228:         if (_amountReceived < _minimumAmountReceived) revert JuiceBuyback_MaximumSlippage();
229: 
230:         // Wrap and transfer the weth to the pool
231:         weth.deposit{value: _amountToSend}();
232:         weth.transfer(address(pool), _amountToSend);
233:     }

If _amountToSend (the positive delta value that indicates the amount of tokens that need to be transferred to the pool) is less than the value sent to the didPay() function (indicating a partial swap), then the implementation simply sends the required amount to the pool, and ignores any potential unused amount.

In this scenario, any unused amount of ETH will be lost in the delegate contract, as it is not consumed during the swap and is not returned to the payer.

Recommendation

There are two alternatives to mitigate the issue. One is to revert the operation if _amountToSend is less than the original intended amount, _data.amount.value, during the callback to uniswapV3SwapCallback(). This will require to encode _data.amount.value in the data variable to make it available during the callback.

The other alternative is to return any unused funds to the payer. If a positive amount of WETH is present after the swap is executed, it can be returned to the original payer by transferring the token at the end of the _swap() function in JBXBuybackDelegate.

uint256 leftover = address(this).balance;
if (leftover > 0) {
  Address.sendValue(_data.payer, leftover);
}

Assessed type

Other

Report

• Non Critical Issues (2)
• Low Issues (4)

Non Critical Issues

<a name="NC-1"></a>[NC-1] Import declarations should import specific symbols

Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file

Instances (17):

File: contracts/JBXBuybackDelegate.sol

4: import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBController3_1.sol";

5: import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleDataSource.sol";

6: import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBPayDelegate.sol";

7: import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBPayoutRedemptionPaymentTerminal3_1.sol";

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

9: import "@jbx-protocol/juice-contracts-v3/contracts/libraries/JBConstants.sol";

10: import "@jbx-protocol/juice-contracts-v3/contracts/libraries/JBFundingCycleMetadataResolver.sol";

11: import "@jbx-protocol/juice-contracts-v3/contracts/libraries/JBTokens.sol";

12: import "@jbx-protocol/juice-contracts-v3/contracts/structs/JBDidPayData.sol";

13: import "@jbx-protocol/juice-contracts-v3/contracts/structs/JBPayParamsData.sol";

15: import "@openzeppelin/contracts/access/Ownable.sol";

16: import "@openzeppelin/contracts/interfaces/IERC20.sol";

18: import "@paulrberg/contracts/math/PRBMath.sol";

20: import "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol";

21: import "@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback.sol";

22: import "@uniswap/v3-core/contracts/libraries/TickMath.sol";

24: import "./interfaces/external/IWETH9.sol";

<a name="NC-2"></a>[NC-2] JBFundingCycleMetadataResolver library is not used and could be removed

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

The JBFundingCycleMetadataResolver library is not used and could be removed, along with the JBFundingCycle import.

Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (1):

File: contracts/JBXBuybackDelegate.sol

2: pragma solidity ^0.8.16;

<a name="L-2"></a>[L-2] Unprotected access to payParams() may allow anyone to set mintedAmount and reservedRate

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

The payParams() function is intended to be called by the JuiceBox terminal, however its access is unprotected and anyone could call it. In particular, this can be used to arbitrarily modify the values of mintedAmount and reservedRate.

<a name="L-3"></a>[L-3] Validate Uniswap pool is using WETH and the project token

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

Ensure the given Uniswap pool is using WETH and the project token as the pool's pair.

<a name="L-4"></a>[L-4] Converting a negative int256 into a positive value may revert

Instances (2):

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

Converting a negative value of variable of type int256 into a positive one by doing -variable will revert the operation if the variable equals type(int256).min as the result overflows the capacity of the type.