The claimRewards() function allows users to claim rewards from the Pendle Market contract. The function performs several operations, including calculating fees and transferring rewards to the user. However, in comparison with other critical functions in the contract, this function lacks the nonReentrant modifier, which could potentially expose the function to reentrancy attacks.

Although current implementation of the function makes external calls to addresses that are expected to be safe, such as the PendleMarket contract and the Holding contract, the risk of reentrancy behavior is present.

Add a nonReentrant modifier to the claimRewards() function to prevent reentrancy behavior.

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

The withdraw() function uses ceiling rounding when calculating the share ratio through OperationsLib.getRatio(). This calculation determines what portion of the user's tokenOut balance should be withdrawn based on the requested shares.

The ceiling rounding in share ratio calculation could allow users to withdraw slightly more assets than they should receive. When performing multiple small withdrawals instead of a single large one, these rounding errors can accumulate. This creates an accounting discrepancy where the total withdrawn amount could exceed the user's actual entitled balance, potentially leading to economic losses for the protocol or other users.

Code Location

params.shareRatio = OperationsLib.getRatio({
    numerator: _shares,
    denominator: recipients[_recipient].totalShares,
    precision: IERC20Metadata(tokenOut).decimals(),
    rounding: OperationsLib.Rounding.Ceil
});

Modify the share ratio calculation to use floor rounding instead of ceiling rounding. When dealing with asset withdrawals, it's safer to round down to ensure users cannot extract more value than their entitled share.

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

The initialize() function lacks proper input validation for contract parameters. The address parameter jigsawRewardToken is not validated against the 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 6173cc1 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-effects-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 e206353 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.

The Natspec documentation for the initialize() function is incorrect, as it refers to the initialize() function of the IonStrategy contract.

Code Location

* @notice Initializes the Ion Strategy contract with necessary parameters.

* - tokenOut: The address of the Ion receipt token (iToken) received as output from the strategy.

Update the Natspec documentation for the initialize() function in the PendleStrategy contract to accurately describe the function's purpose.

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

The call to the addLiquiditySingleToken() function in the deposit() function is expected to return three uint256 type values, which are ignored during the function execution (netLpOut, netSyFee, netSyInterm).

Similarly, the call to the removeLiquiditySingleToken() function in the withdraw() function is expected to return three uint256 type values, which are also ignored (netTokenOut, netSyFee, netSyInterm).

Ignoring return values can lead to unexpected behavior and may result in unanticipated outcomes.

Ensure that the return values of external calls are handled appropriately throughout the contract.

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

The OperationNotSupported error is defined but never used in the contract.

Code Location

error OperationNotSupported();

Remove the unused error declaration.

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