ION Strategies v1 - Jigsaw Finance


Prepared by:

Halborn Logo

HALBORN

Last Updated 02/26/2025

Date of Engagement: October 25th, 2024 - November 6th, 2024

Summary

100% of all REPORTED Findings have been addressed

All findings

20

Critical

0

High

1

Medium

3

Low

4

Informational

12


1. Introduction

Jigsaw Finance engaged Halborn to conduct a security assessment of their ION strategy beginning on October 25th and ending on November 8th. The security assessment was scoped to the smart contracts provided in the GitHub repository.

Commit hash and further details can be found in the Scope section of this report.

2. Assessment Summary

Halborn was provided one week for the engagement and assigned one full-time security engineer to review the security of the smart contract in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.

The purpose of the assessment is to:

    • Identify potential security issues within the smart contracts.

    • Ensure that smart contract functionality operates as intended.

In summary, Halborn identified security findings that were mostly addressed by the Jigsaw Finance team. The main ones were the following: 

    • Address the unintended reward period extension in the addRewards() function.

    • Initialize all inherited upgradeable contracts to prevent errors.

    • Place nonReentrant modifier before other modifiers.

    • Reduce centralization risks by removing concentrated privileges.

    • Modify the withdrawal share ratio calculation to use floor rounding instead of ceiling rounding.


3. Test Approach and Methodology

Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:

    • Research into architecture and purpose.

    • Smart contract manual code review and walkthrough.

    • Graphing out functionality and contract logic/connectivity/functions (solgraph).

    • Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.

    • Manual testing by custom scripts.

    • Static Analysis of security for scoped contract, and imported functions (slither).

    • Testnet deployment (Foundry).

4. RISK METHODOLOGY

Every vulnerability and issue observed by Halborn is ranked based on two sets of Metrics and a Severity Coefficient. This system is inspired by the industry standard Common Vulnerability Scoring System.
The two Metric sets are: Exploitability and Impact. Exploitability captures the ease and technical means by which vulnerabilities can be exploited and Impact describes the consequences of a successful exploit.
The Severity Coefficients is designed to further refine the accuracy of the ranking with two factors: Reversibility and Scope. These capture the impact of the vulnerability on the environment as well as the number of users and smart contracts affected.
The final score is a value between 0-10 rounded up to 1 decimal place and 10 corresponding to the highest security risk. This provides an objective and accurate rating of the severity of security vulnerabilities in smart contracts.
The system is designed to assist in identifying and prioritizing vulnerabilities based on their level of risk to address the most critical issues in a timely manner.

4.1 EXPLOITABILITY

Attack Origin (AO):
Captures whether the attack requires compromising a specific account.
Attack Cost (AC):
Captures the cost of exploiting the vulnerability incurred by the attacker relative to sending a single transaction on the relevant blockchain. Includes but is not limited to financial and computational cost.
Attack Complexity (AX):
Describes the conditions beyond the attacker’s control that must exist in order to exploit the vulnerability. Includes but is not limited to macro situation, available third-party liquidity and regulatory challenges.
Metrics:
EXPLOITABILITY METRIC (mem_e)METRIC VALUENUMERICAL VALUE
Attack Origin (AO)Arbitrary (AO:A)
Specific (AO:S)
1
0.2
Attack Cost (AC)Low (AC:L)
Medium (AC:M)
High (AC:H)
1
0.67
0.33
Attack Complexity (AX)Low (AX:L)
Medium (AX:M)
High (AX:H)
1
0.67
0.33
Exploitability EE is calculated using the following formula:

E=meE = \prod m_e

4.2 IMPACT

Confidentiality (C):
Measures the impact to the confidentiality of the information resources managed by the contract due to a successfully exploited vulnerability. Confidentiality refers to limiting access to authorized users only.
Integrity (I):
Measures the impact to integrity of a successfully exploited vulnerability. Integrity refers to the trustworthiness and veracity of data stored and/or processed on-chain. Integrity impact directly affecting Deposit or Yield records is excluded.
Availability (A):
Measures the impact to the availability of the impacted component resulting from a successfully exploited vulnerability. This metric refers to smart contract features and functionality, not state. Availability impact directly affecting Deposit or Yield is excluded.
Deposit (D):
Measures the impact to the deposits made to the contract by either users or owners.
Yield (Y):
Measures the impact to the yield generated by the contract for either users or owners.
Metrics:
IMPACT METRIC (mIm_I)METRIC VALUENUMERICAL VALUE
Confidentiality (C)None (I:N)
Low (I:L)
Medium (I:M)
High (I:H)
Critical (I:C)
0
0.25
0.5
0.75
1
Integrity (I)None (I:N)
Low (I:L)
Medium (I:M)
High (I:H)
Critical (I:C)
0
0.25
0.5
0.75
1
Availability (A)None (A:N)
Low (A:L)
Medium (A:M)
High (A:H)
Critical (A:C)
0
0.25
0.5
0.75
1
Deposit (D)None (D:N)
Low (D:L)
Medium (D:M)
High (D:H)
Critical (D:C)
0
0.25
0.5
0.75
1
Yield (Y)None (Y:N)
Low (Y:L)
Medium (Y:M)
High (Y:H)
Critical (Y:C)
0
0.25
0.5
0.75
1
Impact II is calculated using the following formula:

I=max(mI)+mImax(mI)4I = max(m_I) + \frac{\sum{m_I} - max(m_I)}{4}

4.3 SEVERITY COEFFICIENT

Reversibility (R):
Describes the share of the exploited vulnerability effects that can be reversed. For upgradeable contracts, assume the contract private key is available.
Scope (S):
Captures whether a vulnerability in one vulnerable contract impacts resources in other contracts.
Metrics:
SEVERITY COEFFICIENT (CC)COEFFICIENT VALUENUMERICAL VALUE
Reversibility (rr)None (R:N)
Partial (R:P)
Full (R:F)
1
0.5
0.25
Scope (ss)Changed (S:C)
Unchanged (S:U)
1.25
1
Severity Coefficient CC is obtained by the following product:

C=rsC = rs

The Vulnerability Severity Score SS is obtained by:

S=min(10,EIC10)S = min(10, EIC * 10)

The score is rounded up to 1 decimal places.
SeverityScore Value Range
Critical9 - 10
High7 - 8.9
Medium4.5 - 6.9
Low2 - 4.4
Informational0 - 1.9

5. SCOPE

Files and Repository
(a) Repository: jigsaw-strategies-v1
(b) Assessed Commit ID: 4b2edc3
(c) Items in scope:
  • src/ion/IonStrategy.sol
  • src/StrategyBaseUpgradeable.sol
  • src/staker/StakerLight.sol
↓ Expand ↓
Out-of-Scope: External libraries, financial-related attacks and test / mock contacts.
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

6. Assessment Summary & Findings Overview

Critical

0

High

1

Medium

3

Low

4

Informational

12

Security analysisRisk levelRemediation Date
Unintended Reward Period Extension in addRewards() FunctionHighSolved - 11/13/2024
Missing Initialization of Inherited Upgradable ContractsMediumSolved - 11/20/2024
Incorrect Order of Modifiers: nonReentrant Should Precede All Other ModifiersMediumSolved - 11/13/2024
Centralization Risk For Trusted OwnersMediumSolved - 11/13/2024
Misuse of initializer Modifier on Internal Initialization FunctionLowSolved - 11/13/2024
Incorrect Rounding Direction in Withdrawal Share CalculationLowSolved - 11/26/2024
Single-step Ownership Transfer ProcessLowSolved - 11/13/2024
Missing Input ValidationLowSolved - 11/13/2024
StakerLightFactory Requires Dual-Step InitializationInformationalSolved - 11/13/2024
Lack of Event EmissionInformationalSolved - 11/13/2024
Owner Can Renounce OwnershipInformationalSolved - 11/13/2024
Inconsistent NatSpec Documentation and Insufficient Test CoverageInformationalSolved - 11/13/2024
Use of Revert Strings Instead of Custom ErrorInformationalAcknowledged - 11/28/2024
Prefer encodeCall() to encodeWithSignature()InformationalSolved - 11/13/2024
Unlocked Pragma CompilerInformationalSolved - 11/26/2024
Consider Using Named MappingsInformationalSolved - 11/13/2024
Magic Numbers in UseInformationalSolved - 11/13/2024
Style Guide OptimizationsInformationalSolved - 11/13/2024
PUSH0 might not be supported in all chainsInformationalAcknowledged - 11/26/2024
Unused Import and Global State VariableInformationalSolved - 11/13/2024

7. Findings & Tech Details

7.1 Unintended Reward Period Extension in addRewards() Function

//

High

Description

The addRewards() function in the StakerLight contract contains a logic flaw that allows for the continuous extension of the periodFinish variable with each call, regardless of whether the current reward period has ended. Specifically, each time addRewards() is called, periodFinish is reset to block.timestamp + rewardsDuration, which effectively "extends" the current reward period if the function is called before the previous period expires.

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 });

    uint256 duration = rewardsDuration;
    if (duration == 0) revert ZeroRewardsDuration();
    if (block.timestamp >= periodFinish) {
        rewardRate = _amount / duration;
    } else {
        uint256 remaining = periodFinish - block.timestamp;
        uint256 leftover = remaining * rewardRate;
        rewardRate = (_amount + leftover) / duration;
    }

    if (rewardRate == 0) revert RewardAmountTooSmall();

    uint256 balance = IERC20(rewardToken).balanceOf(address(this));
    if (rewardRate > (balance / duration)) revert RewardRateTooBig();

    lastUpdateTime = block.timestamp;
    periodFinish = block.timestamp + duration;
    emit RewardAdded(_amount);
}
BVSS
Recommendation

To ensure that the reward period only extends after each complete period, modify the addRewards() function to update periodFinish only if the current timestamp is greater than or equal to the existing periodFinish. This change will prevent mid-period extensions and ensure that the reward period behaves predictably.


Update the relevant part of the addRewards() function as follows:

function addRewards(
    address _from,
    uint256 _amount
) external override onlyOwner validAmount(_amount) updateReward(address(0)) {
...
if (block.timestamp >= periodFinish) {
    rewardRate = _amount / rewardsDuration;
    periodFinish = block.timestamp + rewardsDuration;
} else {
    uint256 remaining = periodFinish - block.timestamp;
    uint256 leftover = remaining * rewardRate;
    rewardRate = (_amount + leftover) / rewardsDuration;
    // Do not update periodFinish if the period has not ended
}
...
Remediation

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

Remediation Hash

7.2 Missing Initialization of Inherited Upgradable Contracts

//

Medium

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.3 Incorrect Order of Modifiers: nonReentrant Should Precede All Other Modifiers

//

Medium

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.4 Centralization Risk For Trusted Owners

//

Medium

Description

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.

BVSS
Recommendation

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.

Remediation

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.

Remediation Hash

7.5 Misuse of initializer Modifier on Internal Initialization Function

//

Low

Description

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.

BVSS
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.

Remediation

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

Remediation Hash

7.6 Incorrect Rounding Direction in Withdrawal Share Calculation

//

Low

Description

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
});
BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.7 Single-step Ownership Transfer Process

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.8 Missing Input Validation

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation

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.

Remediation Hash

7.9 StakerLightFactory Requires Dual-Step Initialization

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.10 Lack of Event Emission

//

Informational

Description

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.

BVSS
Recommendation

All functions updating important parameters should emit events.

Remediation

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.

Remediation Hash

7.11 Owner Can Renounce Ownership

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.12 Inconsistent NatSpec Documentation and Insufficient Test Coverage

//

Informational

Description

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 {

/**
  * @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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.13 Use of Revert Strings Instead of Custom Error

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation

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

7.14 Prefer encodeCall() to encodeWithSignature()

//

Informational

Description

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/

BVSS
Recommendation

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

Remediation

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

Remediation Hash

7.15 Unlocked Pragma Compiler

//

Informational

Description

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.

BVSS
Recommendation

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

Remediation

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

Remediation Hash

7.16 Consider Using Named Mappings

//

Informational

Description

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.

BVSS
Recommendation

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;
Remediation

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

Remediation Hash

7.17 Magic Numbers in Use

//

Informational

Description

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));
  }
BVSS
Recommendation

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).

Remediation

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

Remediation Hash

7.18 Style Guide Optimizations

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation

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

Remediation Hash

7.19 PUSH0 might not be supported in all chains

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation

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

7.20 Unused Import and Global State Variable

//

Informational

Description

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.

BVSS
Recommendation

For better clarity, consider removing all unused code.

Remediation

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

Remediation Hash

8. Automated Testing

Static Analysis Report

Description

Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.

All issues identified by Slither were proved to be false positives or have been added to the issue list in this report.

Output

Slither Output

Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.

© Halborn 2025. All rights reserved.