Prepared by:
HALBORN
Last Updated 02/26/2025
Date of Engagement: October 25th, 2024 - November 6th, 2024
100% of all REPORTED Findings have been addressed
All findings
20
Critical
0
High
1
Medium
3
Low
4
Informational
12
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.
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.
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
).
EXPLOITABILITY METRIC () | METRIC VALUE | NUMERICAL 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 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL 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 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
0
High
1
Medium
3
Low
4
Informational
12
Security analysis | Risk level | Remediation Date |
---|---|---|
Unintended Reward Period Extension in addRewards() Function | High | Solved - 11/13/2024 |
Missing Initialization of Inherited Upgradable Contracts | Medium | Solved - 11/20/2024 |
Incorrect Order of Modifiers: nonReentrant Should Precede All Other Modifiers | Medium | Solved - 11/13/2024 |
Centralization Risk For Trusted Owners | Medium | Solved - 11/13/2024 |
Misuse of initializer Modifier on Internal Initialization Function | Low | Solved - 11/13/2024 |
Incorrect Rounding Direction in Withdrawal Share Calculation | Low | Solved - 11/26/2024 |
Single-step Ownership Transfer Process | Low | Solved - 11/13/2024 |
Missing Input Validation | Low | Solved - 11/13/2024 |
StakerLightFactory Requires Dual-Step Initialization | Informational | Solved - 11/13/2024 |
Lack of Event Emission | Informational | Solved - 11/13/2024 |
Owner Can Renounce Ownership | Informational | Solved - 11/13/2024 |
Inconsistent NatSpec Documentation and Insufficient Test Coverage | Informational | Solved - 11/13/2024 |
Use of Revert Strings Instead of Custom Error | Informational | Acknowledged - 11/28/2024 |
Prefer encodeCall() to encodeWithSignature() | Informational | Solved - 11/13/2024 |
Unlocked Pragma Compiler | Informational | Solved - 11/26/2024 |
Consider Using Named Mappings | Informational | Solved - 11/13/2024 |
Magic Numbers in Use | Informational | Solved - 11/13/2024 |
Style Guide Optimizations | Informational | Solved - 11/13/2024 |
PUSH0 might not be supported in all chains | Informational | Acknowledged - 11/26/2024 |
Unused Import and Global State Variable | Informational | Solved - 11/13/2024 |
//
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);
}
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
}
...
SOLVED: The Jigsaw Finance team solved this finding in commit 357b1bc
by implementing the recommended fix.
//
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.
// 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.
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.
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.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed