The initialize() function lacks proper input validation for contract parameters. Address parameters including rewardToken, jigsawRewardToken, and lendingPool are not validated against zero address. Additionally, the jigsawRewardDuration parameter lacks bounds checking, which could allow initialization with zero value or setting values beyond reasonable thresholds.

Add zero-address validation checks for all address parameters in the initialize() function and proper threshold validation for the jigsawRewardDuration parameter.

SOLVED: The Jigsaw Protocol team solved this finding in commit 14d7ed7 by adding validation checks for all addresses except for the jigsawRewardDuration parameter and stated the following rationale:

This is intentional as we might want to be able to set jigsawRewardDuration equal to zero in some cases. Requiring that jigsawRewardDuration will always not be 0 would break product’s requirements. And in other hand we couldn’t come up with acceptable «upper limit» for input validation in case of jigsawRewardDuration.

The withdraw() function allows the withdrawal of deposited funds from the strategy. However, the function does not follow the checks-effect-interaction pattern, which is a common pattern in Solidity development to prevent reentrancy attacks.

According to this pattern, any modifications to the contract's state should precede calls to external contracts or addresses, but the withdraw() call to the jigsawStaker contract precedes the totalShares and investedAmount state changes in this case. While the function is protected with the nonReentrant modifier, it is always recommended to follow the checks-effect-interaction pattern.

Code Location

// Register `_recipient`'s withdrawal operation to stop generating jigsaw rewards.
jigsawStaker.withdraw({ _user: _recipient, _amount: _shares });

recipients[_recipient].totalShares -= _shares;
recipients[_recipient].investedAmount = params.investment > recipients[_recipient].investedAmount
    ? 0
    : recipients[_recipient].investedAmount - params.investment;

Follow the checks-effects-interactions pattern in the withdraw() function by moving the external call to the jigsawStaker contract to the end of the function.

SOLVED: The Jigsaw Protocol team solved this finding in commit 08a52f3 by following the mentioned recommendation.

The contract in scope currently uses floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.20, and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version to the same version used during development and testing.

SOLVED: The Jigsaw Protocol team solved this finding in commit 29c067b by following the mentioned recommendation.

Throughout the file in scope, there are several instances of use of revert strings over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

For more reference, see here.

ACKNOWLEDGED: The Jigsaw Protocol team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the contract, the return values of the functions are not named. Using named output parameters can improve code readability and make it easier to understand the purpose of each return value, especially when the function has multiple return values.

Use named output parameters in all functions to improve code readability and maintainability, complementing the Natspec documentation.

ACKNOWLEDGED: The Jigsaw Protocol team made a business decision to acknowledge this finding and not alter the contracts.