Juicebox Buyback Delegate - RaymondFam's results

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L235-L239 https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol#L519-L546 https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/JBSingleTokenPaymentTerminalStore3_1.sol#L523-L550

Vulnerability details

Impact

The redeemParams() function referenced in both contracts, i.e. JBSingleTokenPaymentTerminalStore.recordRedemptionFor() and JBSingleTokenPaymentTerminalStore3_1.recordRedemptionFor() currently has no defined logic. This means that when either of the recordRedemptionFor() functions tries to call redeemParams(), no actual computation or operation will be carried out. This is due to redeemParams() being undefined or empty. As a result, it could cause the recordRedemptionFor() functions to not work as expected, or even fail entirely, since they're expecting return values from redeemParams().

This could potentially cause a failure in the token redemption process. The amount of terminal tokens reclaimed, delegate allocations, and memo might not be properly calculated or assigned due to this missing logic. This can result in a serious impact on the functionality of the whole contract and might cause loss of tokens during the redemption process or misallocation of tokens.

Proof of Concept

The issue lies in this segment of the contract code:

https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol#L519-L546 https://github.com/jbx-protocol/juice-contracts-v3/blob/main/contracts/JBSingleTokenPaymentTerminalStore3_1.sol#L523-L550

if (fundingCycle.useDataSourceForRedeem() && fundingCycle.dataSource() != address(0)) {
    // Create the params that'll be sent to the data source.
    JBRedeemParamsData memory _data = JBRedeemParamsData(
      IJBSingleTokenPaymentTerminal(msg.sender),
      _holder,
      _projectId,
      fundingCycle.configuration,
      _tokenCount,
      _totalSupply,
      _currentOverflow,
      _reclaimedTokenAmount,
      fundingCycle.useTotalOverflowForRedemptions(),
      _state == JBBallotState.Active
        ? fundingCycle.ballotRedemptionRate()
        : fundingCycle.redemptionRate(),
      _memo,
      _metadata
    );
    (reclaimAmount, memo, delegateAllocations) = IJBFundingCycleDataSource(
      fundingCycle.dataSource()
    ).redeemParams(_data);
}

The redeemParams() function here is expected to return values, but if it's empty or not properly defined, it will not return the expected values and could lead to unintended behavior:

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

    function redeemParams(JBRedeemParamsData calldata _data)
        external
        override
        returns (uint256 reclaimAmount, string memory memo, JBRedemptionDelegateAllocation[] memory delegateAllocations)
    {}

Tools Used

Manual

Recommended Mitigation Steps

Consider carrying out the following steps before the contract is deployed:

1. The redeemParams() function logic should be properly defined. Make sure it performs the necessary operations and returns the expected values. Ensure it is correctly integrated with the IJBFundingCycleDataSource interface and interacts correctly with the data source.

2. Implement proper error handling: if the redeemParams() function is not defined or fails to execute, the contract should be able to handle this and not proceed with the recordRedemptionFor() function execution. This will prevent unintended behavior and potential token loss.

3. Conduct rigorous unit and integration testing to ensure the redeemParams() function and, by extension, the recordRedemptionFor() function works as expected.

Assessed type

Other

Typo mistakes

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

- *         of project tokens between minting using the project weigh and swapping in a
+ *         of project tokens between minting using the project weight and swapping in a

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

-     * @notice Swap the terminal token to receive the project toke_beforeTransferTon
+     * @notice Swap the terminal token to receive the project token_beforeTransferToken

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

-     * @dev    Slippage controle is achieved here
+     * @dev    Slippage control is achieved here

Unused imports

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

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

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

Contract upgradeability

Ensure there is a provision for contract upgradeability or proxy patterns. If a vulnerability is discovered in the contract after deployment, having an upgrade path is crucial.

Reentrancy attacks

You should check if the contract is vulnerable to reentrancy attacks. This is when an external contract hijacks the control flow, and reenters a contract at a state it wasn’t expecting.

Ownership control

The contract extends from Ownable, make sure only the owner can call certain methods, like changing the pool, projectToken, or jbxTerminal addresses. Nonetheless, it does not seem a need for inheriting Ownable consdering the onlyOwner modifier has not been found appearing in any of the function visibilities.

Parameterized custom errors

Parameterizing custom errors can indeed provide more detailed information when an error is thrown, which can be very beneficial for debugging or for providing better feedback to users.

Custom errors in Solidity can be defined with parameters and they can be used to provide detailed error messages. Here is how you could redefine the JuiceBuyback_Unauthorized and JuiceBuyback_MaximumSlippage errors with additional parameters:

error JuiceBuyback_Unauthorized(address sender, address required);
error JuiceBuyback_MaximumSlippage(uint256 currentSlippage, uint256 maximumSlippage);

// Usage
if (msg.sender != owner) {
    revert JuiceBuyback_Unauthorized({sender: msg.sender, required: owner});
}

if (currentSlippage > maximumSlippage) {
    revert JuiceBuyback_MaximumSlippage({currentSlippage: currentSlippage, maximumSlippage: maximumSlippage});
}

With these parameterized errors, you'll be able to see the address of the sender who attempted an unauthorized operation and which address was required for the operation. Similarly, for the slippage error, you'll see both the current and maximum allowable slippage amounts.

Remember, these custom error messages with parameters are particularly useful during the development and debugging phase but can also provide valuable information to end-users. But be cautious of gas costs when adding too much detail to error messages in a live contract on Ethereum mainnet, as it can become expensive.

Front running attacks

Evaluate the possible scenarios of transaction-ordering dependencies and consider measures to prevent front running.

Input validation

The lack of validation for _data.amount.value and _tokenCount can lead to wastage of gas and execution of futile operations. If _data.amount.value is zero, _tokenCount will also be zero, rendering subsequent function calls, like didPay, ineffective. This could potentially cause logical errors or disruptions in the contract's operations.

The vulnerable code can be located in the mintNFT and didPay functions where _data.amount.value and _tokenCount are used respectively without prior validation. This can be confirmed by reviewing the source code.

Consider implementing validation checks before using _data.amount.value and _tokenCount. This can be done using Solidity modifiers to ensure the values are greater than zero before proceeding. For example:

modifier nonZero(uint _value) {
    require(_value > 0, "Value cannot be zero");
    _;
}

function mintNFT(..., _data.amount.value, ...) public nonZero(_data.amount.value) {...}

function didPay(..., _tokenCount, ...) public nonZero(_tokenCount) {...}

Zero address checks

The contract seems to initialize instances of _projectToken, _weth, _pool, and _jbxTerminal in the constructor. It is crucial to verify that none of these addresses are the zero address.

In Solidity, you can use the require() function to check these conditions and revert the transaction if any of them are not met. Here's how you might validate these addresses in your constructor:

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

constructor(
    address _projectToken,
    address _weth,
    address _pool,
    address _jbxTerminal
) {
    require(_projectToken != address(0), "_projectToken address cannot be 0");
    require(_weth != address(0), "_weth address cannot be 0");
    require(_pool != address(0), "_pool address cannot be 0");
    require(_jbxTerminal != address(0), "_jbxTerminal address cannot be 0");

    projectToken = IERC20(_projectToken);
    weth = IWETH(_weth);
    pool = IUniswapV3Pool(_pool);
    jbxTerminal = IJBXTerminal(_jbxTerminal);
}

In this code, each of the provided addresses is checked to ensure it's not the zero address before assigning it to the corresponding contract instance. The error messages provided in the require statements help to identify which address was zero in case of a failed transaction.

This kind of check ensures that your contract is not initialized with any zero-address dependencies, which would cause your contract functions to fail when trying to interact with these contracts.

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.16",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use a more recent version of solidity

The protocol adopts version 0.8.16 on writing contracts. For better security, it is best practice to use the latest Solidity version, 0.8.19.

Security fix list in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Function order optimization

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Numerical readability

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

    uint256 private constant SLIPPAGE_DENOMINATOR = 10000;

The choice between writing 10000 and 1e4 in code is largely a matter of style and readability. Both are equivalent in the Solidity language and represent the same value.

Using 1e4 may be more readable to some as it concisely expresses the value as "one followed by 4 zeroes" and it's commonly used in scientific notation. However, it can also be less intuitive to others, especially those not familiar with the notation.

On the other hand, 10000 is more straightforward and might be easier to understand at a glance, especially for those who are not as familiar with the exponential notation. However, for larger numbers, this method can become hard to read.

Considering these aspects, you might want to make the choice based on who will be reading and maintaining the code. You could also follow the existing style in the rest of your code base for consistency.

In general, it's always good to have comments in the code to explain any magic numbers (constants), regardless of how they're written, just as how it has been implemented here.