The StrategyBaseUpgradeable contract inherits from OwnableUpgradeable, ReentrancyGuardUpgradeable and UUPSUpgradeable, but only calls the initializers for OwnableUpgradeable (__Ownable_init) and UUPSUpgradeable (__UUPSUpgradeable_init) within its __StrategyBase_init() function. This leaves ReentrancyGuardUpgradeable uninitialized.

The StakerLight contract inherits from OwnableUpgradeable and ReentrancyGuardUpgradeable, but only calls the initializer for OwnableUpgradeable (__Ownable_init) within its initialize() function. This leaves ReentrancyGuardUpgradeable uninitialized.

In the OpenZeppelin upgradeable contract pattern, initializers are used to set up state variables and other configurations that would typically be done in a constructor for non-upgradeable contracts. Failing to explicitly call the __ReentrancyGuard_init() initializer means that any future changes to the ReentrancyGuardUpgradeable contract's initialization logic will not be applied to this contract.

It is recommended to initialize all inherited upgradable contracts. More specifically, explicitly call the __ReentrancyGuard_init() function in the initialize functions to ensure all inherited modules are properly initialized. This approach aligns with standard practices for using the OpenZeppelin upgradeable pattern and helps future-proof the contract against potential changes in the underlying library.

SOLVED: The Jigsaw Finance team solved this finding in commits a18a7fc, b9c63da, 7dbfa3d by implementing the recommended fix.

To mitigate the risk of reentrancy attacks, a modifier named nonReentrant is commonly used. This modifier acts as a lock, ensuring that a function cannot be called recursively while it is still in execution. A typical implementation of the nonReentrant modifier locks the function at the beginning and unlocks it at the end. However, it is critical to place the nonReentrant modifier before all other modifiers in a function. Placing it first ensures that all other modifiers cannot bypass the reentrancy protection. In the current implementation, some functions use other modifiers before nonReentrant, which compromises the protection it provides.

For example, in the deposit() function below, the nonReentrant is placed after two other modifier calls, a reentrancy attack could potentially bypass the lock and manipulate the contract by exploiting the privileges of the owner:

function deposit(
  address _asset,
  uint256 _amount,
  address _recipient,
  bytes calldata _data
) external override onlyValidAmount(_amount) onlyStrategyManager nonReentrant returns (uint256, uint256) {

The risk of this finding was increased to medium risk after reviewing that the onlyStrategyManager modifier of the deposit() function of the IonStrategy contract is interacting with other addresses.

By following the best practice of placing the nonReentrant modifier before all other modifiers, one can significantly reduce the risk of reentrancy-related vulnerabilities. This is simple yet effective approach can help augment the security posture of any Solidity smart contract.

SOLVED: The Jigsaw Finance team solved this finding in commit 01e25cc by implementing the recommended fix.

The StakerLight contract grants extensive control to the contract owner, including the authority to pause and unpause the contract, set reward amounts and durations, adjust withdrawal periods, and perform emergency token withdrawals. While these functions can be essential for contract management and maintenance, they introduce significant centralization risks that could potentially be exploited or misused.

Here are two critical functions controlled by the owner in detail:

• addRewards() function:

function addRewards(
    address _from,
    uint256 _amount
) external override onlyOwner validAmount(_amount) updateReward(address(0)) {
  // To mitigate any DOS issues, Admin must deposit 1 wei into the staker contract at the initialization
  require(_totalSupply != 0, "Zero totalSupply");

  // Transfer assets from the user's wallet to this contract.
  IERC20(rewardToken).safeTransferFrom({ from: _from, to: address(this), value: _amount });
  ...

This function allows the owner to add rewards by transferring tokens from any _from address that has approved the StakerLight contract to spend its rewardToken. This creates a potential risk where the owner could invoke addRewards() using any address with an active approval, moving tokens without that user’s immediate consent or awareness.

• recoverERC20() function:

function recoverERC20(address tokenAddress, uint256 tokenAmount) external override onlyOwner {
    IERC20(tokenAddress).safeTransfer(owner(), tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

This function allows the owner to recover ERC20 tokens from the contract and transfer them to their own address. This capability, especially when combined with the control over the addRewards() function, poses a risk that the owner could call addRewards() to pull approved tokens from any user and subsequently use recoverERC20() to withdraw those tokens to their personal wallet. The owner could exploit the addRewards() function to transfer tokens from a user who has unknowingly approved the contract, followed by calling recoverERC20() to move those tokens out of the contract for personal gain.

To mitigate these risks, consider the following:

• Multi-Signature Controls: Implement multi-signature requirements for sensitive operations to distribute control and prevent unilateral decisions.

• Governance Process: Introduce a governance mechanism for approving significant changes and recoveries to involve the community or other stakeholders.

• Use of Time-Locks: Incorporate time-locks for critical functions to add a delay before execution. This delay allows stakeholders to review and potentially react to suspicious or harmful actions before they are finalized.

• Review the Necessity of _from in addRewards(): Assess whether the _from argument in addRewards() is necessary. If the goal is simply to add rewards from the owner, removing this argument would prevent the owner from accessing tokens from other approved users.

• Restrict recoverERC20() Scope: Limit the recoverERC20() function to only allow the recovery of non-critical tokens that do not interfere with the contract’s main operations. This restriction would reduce the potential for misuse.

SOLVED: The Jigsaw Finance team solved this finding in commit f2046af by implementing the recommended fix and removing the _from argument from the addRewards(). It is also worth mentioning that it is still recommended to consider the other mitigations enumerated in this finding, like the use of multi-signature wallets and time-locks.

In IonStrategy.sol, the initialize() function calls an internal function, __StrategyBase_init(), from StrategyBaseUpgradeable.sol. Both initialize() and __StrategyBase_init() are marked with the initializer modifier. This setup can lead to issues because calling multiple functions with initializer during the same initialization sequence may cause conflicts, ambiguous behavior, or unintended reinitialization errors in upgradeable contracts.

When initialize() in IonStrategy is called, it executes __StrategyBase_init() within the same call stack. In OpenZeppelin’s upgradeable framework, initializer typically marks the contract as initialized only at the end of the call, which allows the nested __StrategyBase_init() call to succeed on the first execution.

For internal initialization functions, the best practice is to use the onlyInitializing modifier, as detailed in the recommendation.

To address this issue, use the onlyInitializing modifier on __StrategyBase_init() instead of initializer. This approach ensures that __StrategyBase_init() can only be called during initialization but does not independently mark the contract as initialized. This change will:

• Allow __StrategyBase_init() to be safely called from the main initialize() function in IonStrategy without marking the contract as initialized.

• Enable consistent, predictable initialization flows if IonStrategy is inherited by other contracts.

• Reduce the risk of reinitialization conflicts or ambiguous initialization behavior.

Here’s how the implementation can be improved:

function __StrategyBase_init(address _initialOwner) internal onlyInitializing {

With this change, initialize() in IonStrategy can safely call __StrategyBase_init() during initialization, while avoiding reinitialization risks and improving code clarity.

SOLVED: The Jigsaw Finance team solved this finding in commit 1827403 by implementing the recommended fix.

The withdraw() function uses ceiling rounding when calculating the share ratio through OperationsLib.getRatio(). This calculation determines what portion of the user's aToken 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

// Calculate the ratio between all user's shares and the amount of shares used for withdrawal.
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 Finance team solved this finding in commit 481aacb by implementing the recommended fix.

It was identified that the IonStrategy, StrategyBaseUpgradeable and StakerLight contracts inherited from OpenZeppelin's OwnableUpgradeable library. Ownership of the contracts that are inherited from the OwnableUpgradeable module can be lost, as the ownership is transferred in a single-step process.

The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2StepUpgradeable is safer than OwnableUpgradeable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2StepUpgradeable. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2StepUpgradeable provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2StepUpgradeable, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

SOLVED: The Jigsaw Finance team solved this finding in commit f98dd68 by implementing the recommended fix.

During the security assessment, it was identified that some functions in the smart contracts lack proper input validation, allowing critical parameters to be set to undesired or unrealistic values. This can lead to potential vulnerabilities, unexpected behavior, or erroneous states within the contract.

Examples include, but are not limited to:

• No address(0) check on initialize() in src/staker/StakerLight.sol.

• No minimums or maximum values on setRewardsDuration() or initialize() in src/staker/StakerLight.sol.

This list is not exhaustive. It is recommended to conduct a comprehensive review of the codebase to identify and assess other functions that may require additional input validation. Ensuring appropriate checks are in place for critical parameters will enhance the overall reliability, security, and predictability of the contracts.

To mitigate these issues, implement input validation in all constructor functions and other critical functions to ensure that inputs meet expected criteria. This can prevent unexpected behaviors and potential vulnerabilities.

SOLVED: The Jigsaw Finance team solved this finding in commit 1ffa561 by implementing the recommended fix. Consideration should be given to look for more instances of missing input validation to avoid any potential unexpected behavior.

The StakerLightFactory contract, as implemented, has a critical dependency on setting the referenceImplementation before its primary function, createStakerLight(), can be invoked. This dependency creates a mandatory two-step initialization process that effectively renders the factory non-functional until the setStakerLightReferenceImplementation() function is called by the owner. This design results in operational inefficiency, potential downtime, and increased gas costs due to the need for an additional external transaction post-deployment.

The factory contract could instead set the referenceImplementation directly in the constructor, eliminating the need for a secondary setup call and ensuring that the contract is immediately operational upon deployment:

contract StakerLightFactory is IStakerLightFactory, Ownable2Step {
...

constructor(
    address _initialOwner
) Ownable(_initialOwner) { }

...

function createStakerLight(
  address _initialOwner,
  address _holdingManager,
  address _rewardToken,
  address _strategy,
  uint256 _rewardsDuration
) external override returns (address newStakerLightAddress) {
  // Assert that referenceImplementation has code in it to protect the system from cloning invalid implementation.
  require(referenceImplementation.code.length > 0, "Reference implementation has no code");
  ...
  }
}

Without setting the referenceImplementation in the constructor, the factory remains non-functional after deployment until an additional call to setStakerLightReferenceImplementation() is made. This results in:

• Operational Downtime: The factory cannot create new StakerLight contracts until it is initialized, leading to a non-functional state post-deployment.

• Additional Gas Costs: Deploying the contract and then calling setStakerLightReferenceImplementation() requires two separate transactions, incurring extra gas fees.

Refactor the constructor to accept an initial referenceImplementation address parameter and set it during deployment. This ensures that the factory is immediately usable after deployment and avoids the need for a secondary transaction, reducing both downtime and gas costs.

SOLVED: The Jigsaw Finance team solved this finding in commit 43b5ab1 by implementing the recommended fix.

It has been observed that some functionalities are missing emitting events.

Events are a method of informing the transaction initiator about the actions taken by the called function. It logs its emitted parameters in a specific log history, which can be accessed outside of the contract using some filter parameters. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

Examples:

• initialize() in src/ion/IonStrategy.sol

• initialize() in src/staker/StakerLight.sol

• setStakerLightReferenceImplementation() in staker/StakerLightFactory.sol

This list is not exhaustive, and it is recommended to review the entire codebase to identify additional instances where event emissions may be beneficial. A thorough analysis should be conducted to determine whether adding event emissions aligns with the intended design and provides value in tracking state changes.

All functions updating important parameters should emit events.

SOLVED: The Jigsaw Finance team solved this finding in commit e3d55c7 by implementing the recommended fix, adding event emissions. However, consideration should be given to look for further instances and improve the event emission of the whole protocol.

It was identified that the StakerLightFactory contract inherited from OpenZeppelin's Ownable2Step library. In the Ownable2Step contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions:

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

Furthermore, the contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

SOLVED: The Jigsaw Finance team solved this finding in commit 3296cf4 by implementing the recommended fix.

Several instances in the codebase exhibit inconsistencies in NatSpec documentation and demonstrate incomplete test coverage. Accurate NatSpec comments are critical for clear and precise contract documentation, aiding developers and auditors in understanding contract functionalities and inheritance. Inaccurate or outdated documentation can lead to misunderstandings and potential integration issues.

Below are examples of NatSpec inconsistencies:

• Misleading Inheritance Documentation in StakerLight Contract: The following NatSpec comment suggests inheritance from Ownable2Step and ReentrancyGuard, whereas the actual inheritance is from OwnableUpgradeable and ReentrancyGuardUpgradeable:

 * @dev This contract inherits functionalities from `Ownable2Step` and `ReentrancyGuard`.
 *
 * @author Hovooo (@hovooo)
 */
contract StakerLight is IStakerLight, OwnableUpgradeable, ReentrancyGuardUpgradeable {

• Outdated Modifier Comment: The comment for the onlyStrategy modifier appears to be copied and pasted from an external source (https://github.com/jigsaw-finance/jigsaw-lite/blob/c583849a52591b8f94ddaf3761a5d6c808e8f9fb/src/Staker.sol#L119-L126), leading to a potentially misleading description:

/**
  * @notice Modifier to restrict a function to be called only by the staking manager.
  * @notice Reverts the transaction if the caller is not the staking manager.
  */
modifier onlyStrategy() {
    if (msg.sender != strategy) revert UnauthorizedCaller();
    _;
}

• Incorrect Constructor Documentation in StakerLightFactory: The constructor's NatSpec indicates that it creates a StablesManager contract, which is incorrect:

contract StakerLightFactory is IStakerLightFactory, Ownable2Step {
...
    /**
     * @notice Creates a new StablesManager contract.
     * @param _initialOwner The initial owner of the contract.
     */
    constructor(
...

• Incomplete Parameter Documentation in IonStrategy: The initialize() function in the IonStrategy contract lacks documentation for its parameters:

/**
  * @notice Initializer for the Ion Strategy.
  */
function initialize(
  InitializerParams memory _params
) public initializer {

This list may not be exhaustive. It is recommended to perform a broader review of the codebase for other potential instances of inaccurate NatSpec documentation and assess if corrections are necessary.

Additionally, the test suite's coverage is inadequate, missing thorough validation of function behaviors and overall contract performance. This deficiency in testing can lead to undetected bugs and unintended behaviors.

Review the codebase to identify other instances of inaccurate or outdated NatSpec comments and ensure all public and external functions have detailed, accurate documentation. Complement these efforts with a comprehensive analysis of the test suite to expand coverage and ensure robust verification of the contracts' behaviors.

SOLVED: The Jigsaw Finance team solved this finding in commit 1ffa561 and 2642246 by implementing the recommended fix.

In Solidity smart contract 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.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. Use the Custom Error in require or if statements.

Using custom errors inside require statements is only available from Solidity versions 0.8.26 and above. In case of older compiler version, consider using if statements + revert with Custom Error.

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

The external function withdraw() of the IonStrategy contract is making use of the abi.encodeWithSignature() function. Since Solidity version 0.8.11, abi.encodeCall() provides a type-safe encoding utility compared to abi.encodeWithSelector() or abi.encodeWithSignature().

For more information, see: https://solidity-by-example.org/abi-encode/

Consider changing the use abi.encodeWithSignature() for abi.encodeCall() in the external function withdraw() of the IonStrategy contract.

SOLVED: The Jigsaw Finance team solved this finding in commit 7bf63b8 and 8d2d7e3 by implementing the recommended fix.

The files in scope currently use 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. 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 Finance team solved this finding in commit 6e77373 by implementing the recommended fix.

The project is using a Solidity version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

Furthermore, the currentMultiplier() function's readability would improve if it used named returns.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

For example, on StakerLight.sol, instead of declaring:

mapping(address => uint256) public override userRewardPerTokenPaid;

It could be declared as:

mapping(address user => uint256 amountPaid) public override userRewardPerTokenPaid;

SOLVED: The Jigsaw Finance team solved this finding in commit f93d435 by implementing the recommended fix.

In programming, magic numbers refers to the use of unexplained numerical or string values directly in code, without any clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make your code more difficult to understand, maintain, and update.

To improve the readability and maintainability of your smart contracts, it is recommended to avoid using magic numbers and instead use named constants or variables to represent these values. By doing so, you provide clear context for the values, making it easier for developers to understand their purpose and significance.

An example of magic number usage was found in the _mint() function within StrategyBaseUpgradeable.sol, notice the use of the magic number 18 below:

function _mint(IReceiptToken _receiptToken, address _recipient, uint256 _amount, uint256 _tokenDecimals) internal {
  uint256 realAmount = _amount;
  if (_tokenDecimals > 18) {
    realAmount = _amount / (10 ** (_tokenDecimals - 18));
  } else {
    realAmount = _amount * (10 ** (18 - _tokenDecimals));
  }

To improve code maintainability, readability, and reduce the risk of potential errors, it is recommended to replace magic numbers with well-defined constants. By using constants, developers can provide clear and descriptive names for specific values, making the code easier to understand and maintain. Additionally, updating the values becomes more straightforward, as changes can be made in a single location, reducing the risk of errors and inconsistencies. For large numbers, consider using scientific notation (e.g., 1e4).

SOLVED: The Jigsaw Finance team solved this finding in commit 30c0a8c by implementing the recommended fix.

In Solidity development, adhering to the official style guide is best practice to ensure code consistency, readability, and maintainability. Throughout the contracts, there are several instances where the code does not follow these guidelines. Some examples include:

• The constant totalSupplyLimit in the StakerLight contract is in camelcase.

• The IIonPool interface is defined in the same file as the IonStrategy contract (IonStrategy.sol).

• The deposit() and withdraw() functions in src/ion/IonStrategy.sol are complimentary and have similar arguments, but are ordered inconsistently. This inconsistency reduces readability and may increase the risk of implementation errors.

This list is not exhaustive, and it is recommended to review the entire codebase to identify other areas where style guide improvements can be made.

It is recommended to apply the following style improvements:

• Modify the constant totalSupplyLimit to be in uppercase (TOTAL_SUPPLY_LIMIT).

• Interfaces should be defined in separate files.

• In src/ion/IonStrategy.sol, standardize the argument order between deposit() and withdraw() to follow a consistent structure, such as (asset, amount, recipient, data). This change will enhance readability and help avoid potential errors, especially for developers working with both functions simultaneously.

Implementing these style optimizations will improve the code's readability, consistency, and maintainability, aligning it with Solidity best practices.

SOLVED: The Jigsaw Finance team solved this finding in commit 396ecb98 and 2aea8c3 by implementing the recommended fix.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

ACKNOWLEDGED: The Jigsaw Finance team indicated that they decided that not to deploy our contracts to the chains that will not support PUSH0 opcode.

During the security assessment of the smart contract, an unused import and an unused global state variable were identified:

• Unused Import: The following import is defined in the StrategyBaseUpgradeable.sol file, but is not utilized in the contract code:

import { IStrategy } from "@jigsaw/src/interfaces/core/IStrategy.sol";

• Unused global state variable: The rewardToken global state variable was declared in IonStrategy, but never used. Notice it is using the override keyword because the contract inherits from IStrategy. However, it is never used in the contact, so it always returns address(0):

/**
  * @notice The reward token offered to users.
  */
address public override rewardToken;

Removing unused imports and global state variables can streamline the code, reduce deployment costs, and improve readability.

For better clarity, consider removing all unused code.

SOLVED: The Jigsaw Finance team solved this finding in commit f56d8c9 by implementing the recommended fix.