Juicebox Buyback Delegate - Dimagu's results

C4 Audit Juicebox JBXBuybackDelegate (May 22, 2023)

QA Report (with low) Submission

Low Risk

[L-1] Check _slippage is in specified range and _quote is a valid quote JBXBuybackDelegate.payParams in line 153

Currently, uint256 _slippage and _quotevariable is not sanitized, and an arbitrary value could be assigned to it through _data.metadata.

This could lead to the follwoing unexpected behavior:

1. _slippage to be greater than SLIPPAGE_DENOMINATOR, which will lead to _slippage / SLIPPAGE_DENOMINATOR > 1, and the equation: _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR) will cause an arithmetic underflow
2. _slipage can be arbitrarly set to 0, which might lead to miscalculations of either the protocol should mint or swap tokens
3. It is a better idea to fetch the _quote for the token pair inside JBXBuybackDelegate.payParams instead of passing it through jbEthPaymentTerminal.pay() function through the _metadata. Currently, arbitrary value can be passed for _quote which might lead to to miscalculations of either the protocol should mint or swap tokens.

[L-2] Function `payParams(JBPayParamsData calldata _data)' in line 144 should be protected with an access control

As anyone can call this function, a malicious user can set the value of the 2 mutex _mintedAmount and _reservedRateto 0, making every call much more expensive (cold variable access)

Quality Assurance (QA)

[QA-1] Unnecessary Tuple for decoding one value

(uint256 \_minimumAmountReceived)in line 221 is presented as a tuple, but it is a single value. The parenthesis are not necessary.

[QA-2] Compilation breaks with solidity versions 0.8.16/17/18

UniswapV3ForgeQuoter.solreferenced in package.json line 13 is written in solidity compiler version 0.8.19, which causes a compilation in solidity versions 0.8.16/17/18 to fail.

[QA-3] Unnecessary imports and inherited contracts

1. import "@jbx-protocol/juice-contracts-v3/contracts/interfaces/IJBFundingCycleBallot.sol"; is not needed, line 8
2. import "@openzeppelin/contracts/access/Ownable.sol"; is not needed, line 15
3. There is no need for JBXBuybackDelegate contract to inherit Ownable contract, line 39

[QA-4] Underscore in constant assignment missing, line 68

SLIPPAGE_DENOMINATOR = 10000 should be changed to SLIPPAGE_DENOMINATOR = 10_000 for improved readability.

[QA-5] Typos in comments:

1. Line 32: weigh should be changed to weight
2. Line 102 & 199: typo prefered should be changed to preferred
3. Line 187: Retrieve the number of token created should be changed to Retrieve the number of tokens created
4. Line 204: If swap failed, mint instead, with the original weight + add to balance the token in should be changed to If swap failed, mint instead with the original weight + add to balance the token in
5. Line 212: (where token transfer should happens) should be changed to (where token transfer should happen)
6. Line 214: Slippage controle is achieved here should be changed to Slippage control is achieved here
7. Line 246: Swap the terminal token to receive the project toke_beforeTransferTon incomprehensible comment
8. Line 248: This delegate first receive the whole amount of project token should be changed to This delegate first receives the whole amount of project tokens
9. Line 250: then burn the rest of this delegate balance (ie the amount of reserved token) should be changed to then burn the rest of this delegate balance (ie the amount of reserved tokens)
10. Line 288: If there are reserved token... should be changed to If there are reserved tokens...
11. Line 292: Burn all the reserved token... should be changed to Burn all the reserved tokens...
12. Line 301: Mint the reserved token with... should be changed to Mint the reserved tokens with...
13. Line 311 Burn the non-reserve token... should be changed to Burn the non-reserve tokens...
14. Line 332: the amount of token out to mint should be changed to the amount of tokens out to mint

[QA-6] Keep consistent specific naming for the comments/variables/functions:

1. Line 53 rename the 3rd event parameter from amountOut to amountJBX
2. Line 84 instead of projectToken use JBXToken
3. Line 87 instead of The uniswap pool corresponding to the project token - other token market use The uniswap pool corresponding to the JBX token - wETH market
4. Line 95 instead of jbxTerminal use jbEthPaymentTerminal

[QA-7] Use more generalised titles:

1. Line 57 instead of private constant properties use private constant/immutable properties
2. Line 76 instead of public constant properties use public constant/immutable properties

C4 Audit Juicebox JBXBuybackDelegate (May 22, 2023)

Gas Optimizations Submission

[G-1] Line 267: Arithmetic that is done on the fly can be stored as a constant.

Instead of calculating TickMath.MAX_SQRT_RATIO - 1 and TickMath.MIN_SQRT_RATIO + 1 in line 267 every time swap is conducted, we can define this values as a constant variables. Ex. uint160 private constant MAX_SQRT_RATIO_MINUS_ONE = 1461446703485210103287273052203988822378723970341; & uint160 private constant MIN_SQRT_RATIO_PLUS_ONE = 4295128740;, and we can get rid of import "@uniswap/v3-core/contracts/libraries/TickMath.sol"; library.

Gas Savings for JBXBuybackDelegate.didPay, obtained via protocol's tests: Avg 838 gas

File: src/EthRouter.sol
22:     import "@uniswap/v3-core/contracts/libraries/TickMath.sol";
​
258:    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
259:        internal
260:        returns (uint256 _amountReceived)
261:    {
262:        // Pass the token and min amount to receive as extra data
263:        try pool.swap({
264:            recipient: address(this),
265:            zeroForOne: !_projectTokenIsZero,
266:            amountSpecified: int256(_data.amount.value),
267:            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
268:            data: abi.encode(_minimumReceivedFromSwap)
269:        }) returns (int256 amount0, int256 amount1) {
270:            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
271:            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));
272:        } catch {
273:            // implies _amountReceived = 0 -> will later mint when back in didPay
274:            return _amountReceived;
275:        }
276:
277:        // The amount to send to the beneficiary
278:        uint256 _nonReservedToken = PRBMath.mulDiv(
279:            _amountReceived, JBConstants.MAX_RESERVED_RATE - _reservedRate, JBConstants.MAX_RESERVED_RATE
280:        );
281:
282:        // The amount to add to the reserved token
283:        uint256 _reservedToken = _amountReceived - _nonReservedToken;
284:
285:        // Send the non-reserved token to the beneficiary (if any / reserved rate is not max)
286:        if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken);
287:
288:        // If there are reserved token, add them to the reserve
289:        if (_reservedToken != 0) {
290:            IJBController controller = IJBController(jbxTerminal.directory().controllerOf(_data.projectId));
291:
292:            // 1) Burn all the reserved token, which are in this address -> result: 0 here, 0 in reserve
293:            controller.burnTokensOf({
294:                _holder: address(this),
295:                _projectId: _data.projectId,
296:                _tokenCount: _reservedToken,
297:                _memo: "",
298:                _preferClaimedTokens: true
299:            });
300:
301:            // 2) Mint the reserved token with this address as beneficiary -> result: _amountReceived-reserved here, reservedToken in reserve
302:            controller.mintTokensOf({
303:                _projectId: _data.projectId,
304:                _tokenCount: _amountReceived,
305:                _beneficiary: address(this),
306:                _memo: _data.memo,
307:                _preferClaimedTokens: false,
308:                _useReservedRate: true
309:            });
310:
311:            // 3) Burn the non-reserve token which are now left in this address (can be 0) -> result: 0 here, reservedToken in reserve
312:            uint256 _nonReservedTokenInContract = _amountReceived - _reservedToken;
313:
314:            if (_nonReservedTokenInContract != 0) {
315:                controller.burnTokensOf({
316:                    _holder: address(this),
317:                    _projectId: _data.projectId,
318:                    _tokenCount: _nonReservedTokenInContract,
319:                    _memo: "",
320:                    _preferClaimedTokens: false
321:                });
322:            }
323:        }
324:
325:        emit JBXBuybackDelegate_Swap(_data.projectId, _data.amount.value, _amountReceived);
326:    }

​

@@ -19,7 +19,6 @@ import "@paulrberg/contracts/math/PRBMath.sol";
​
import "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol";
import "@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback.sol";
-import "@uniswap/v3-core/contracts/libraries/TickMath.sol";
​
import "./interfaces/external/IWETH9.sol";
​
@@ -67,6 +66,16 @@ contract JBXBuybackDelegate is IJBFundingCycleDataSource, IJBPayDelegate, IUnisw
     */
    uint256 private constant SLIPPAGE_DENOMINATOR = 10000;
​
+    /**
+     * @notice Uniswap's TickMath.MAX_SQRT_RATIO - 1
+     */
+    uint160 private constant MAX_SQRT_RATIO_MINUS_ONE = 1461446703485210103287273052203988822378723970341;
+
+    /**
+     * @notice Uniswap's TickMath.MIN_SQRT_RATIO + 1
+     */
+    uint160 private constant MIN_SQRT_RATIO_PLUS_ONE = 4295128740;
+
    //*********************************************************************//
    // --------------------- public constant properties ------------------ //
    //*********************************************************************//
@@ -264,7 +273,7 @@ contract JBXBuybackDelegate is IJBFundingCycleDataSource, IJBPayDelegate, IUnisw
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
-            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
+            sqrtPriceLimitX96: _projectTokenIsZero ? MAX_SQRT_RATIO_MINUS_ONE : MIN_SQRT_RATIO_PLUS_ONE,
            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)

[G-2] Use unchecked to save gas

We checked several lines of code where arithmetic is done that we proofed to be more gas efficient when using unchecked.

1. Line 156 to 167 can be rewritten to save gas:

    if (_slippage > SLIPPAGE_DENOMINATOR) {
      revert JuiceBuyback_InvalidSlippage();
    }
    uint256 qs = _quote * _slippage;
    unchecked {
      // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
      if (_tokenCount < _quote - (qs / 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);
      }
    }

Gas Savings for JBXBuybackDelegate.payParams, obtained via protocol's tests: Avg 84 gas

2. Line 197 can be rewritten to save gas:

    if (_slippage > SLIPPAGE_DENOMINATOR) {
      revert JuiceBuyback_InvalidSlippage();
    }
    uint256 qs = _quote * _slippage;
    uint256 _minimumReceivedFromSwap;
    unchecked {
      _minimumReceivedFromSwap = _quote - (qs / SLIPPAGE_DENOMINATOR);
    }

Gas Savings for JBXBuybackDelegate.didPay, obtained via protocol's tests: Avg 78 gas

3. Line 278 - 280 can be rewritten to save gas:

    if (_reservedRate > JBConstants.MAX_RESERVED_RATE) revert JuiceBuyback_InvalidReserveRate();
    uint256 _nonReservedRate;
    unchecked {
       _nonReservedRate = JBConstants.MAX_RESERVED_RATE - _reservedRate;
    }

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

Gas Savings for JBXBuybackDelegate.didPay, obtained via protocol's tests: Avg 15 gas