Jigsaw Lite - Jigsaw Finance

addRewards lacks of any tool to confirm that available funds are assigned to a certain rewardPeriod or still pending to transfer/assign. This, in certain situations, can lead to miscalculations of the rewardRate.

Although this issue can be solved in most situations by adding funds to Staker contract as soon as possible, failure to update accurate reward calculations could lead to:

1. Loss of user trust and decreased participation.

2. Negative impact on reputation and potential legal risks.

3. Economic imbalance within the protocol, causing dissatisfaction among users.

There are only 3 steps:

• Owner of the contract transfer rewardTokens the Staker contract.

• Owner of the contract call addRewards in order to distribute rewards among existing and incoming users, with rewardsDuration = 1 year.

• 6 months later, owner call to addRewards again but this time he decide to only add the half of what was already added at the begining. So calculation of rewardRate starts:

  • Lower limit is defined to be above 0:

    - Calculated leftover is the half of initial. (because the half of rewardsDuration already passed)

    - As _amount, we again add the half of the initial amount, so (leftover + _amount) == initial amount.

    - Duration hasn't change.

    - So finally, rewardRate stays the same.

  • For upper limit we get that rewardToken balance didn't change:

    - Nobody withdrawn rewards since the beginning so apparently there are enough tokens to share, the initial amount.

  • Finally Stacker contract:

    • Renews rewardRate as same value.

    • Renews periodFinish by adding 1 year to current timestamp.

• Consequence:

  • As no rewardToken has been actually added to the contract, It will run out of funds when day 365 (from the beginning) arrives.

  • Last 6 months rewards will be virtually accrued but, if not detected and solved, unavailable in the contract.

function test_addRewards_withoutTransferingRewardTokens() public {
    uint256 amount2add = rewardsDuration * 0.01 ether;

    vm.prank(OWNER, OWNER);
    bool approved = JigsawPoints(rewardToken).approve(address(staker), type(uint256).max);
    require(approved, "approval failed");

    vm.prank(OWNER, OWNER);
    JigsawPoints(rewardToken).transfer(address(staker), amount2add);

    console.log("Add rewards by same amount");
    vm.prank(OWNER, OWNER);
    staker.addRewards(amount2add);

    uint256 initial_rewardRate = staker.rewardRate();

    console.log("6 months later...");
    skip(365 days / 2);
    assertEq(block.timestamp, 365 days / 2 + 1);

    console.log("We add more rewards but tokens has ever been transferred");
    console.log("(in either direction)");
    vm.prank(OWNER, OWNER);
    staker.addRewards(amount2add / 2);

    uint256 final_rewardRate = staker.rewardRate();
    uint256 balance_ = JigsawPoints(rewardToken).balanceOf(address(staker));
    assertEq(final_rewardRate, initial_rewardRate);
    assertEq(staker.periodFinish(), 365 days * 1.5 + 1);
}

It appears that the easiest way to handle this issue is to do actual transferFrom with the same _amount used to call the function to get the rewardTokens into the contract atomically, this way, anytime a reward is added, it will be actually present in the contract.

Remediation Plan

SOLVED: The Jigsaw team introduced the aforementioned transferFrom inside addRewards function.

Functions withdraw and claimRewards, have onlyStakingManager access modifier, but those functions are not implemented in contract StakingManager to be directly called. Therefore, its visibility can be changed to internal.

Change visibility to internal or implement functions to call these from StakingManager contract.

Remediation Plan

SOLVED: The Jigsaw team solved this issue by turning both functions to internal, making several changes accordingly.

AccessControlDefaultAdminRules's constructor is already checking that provided _admin address is different from address zero.
Having two modifiers in a Solidity method that essentially perform the same task of verifying that an address is not address(0) introduces inefficiency and redundancy into the codebase. This redundancy increases the complexity of the code and adds unnecessary overhead to the execution of the method. Maintaining multiple modifiers with duplicate functionality complicates code readability and may lead to confusion for developers trying to understand the purpose and behavior of the method. Additionally, redundant modifiers consume additional gas during contract execution, potentially impacting the overall cost and performance of transactions.

Consider deleting validAddress(_admin) modifier.

Remediation Plan

SOLVED: The Jigsaw team moved from AccessControl to Ownable and suppressed the modifier redundancy.

Utilizing the checks-effects-interactions pattern remains crucial even when a function incorporates the nonReentrant modifier. While the nonReentrant modifier effectively prevents reentrancy attacks by restricting multiple calls to the same function during execution, the checks-effects-interactions pattern provides additional security measures. This pattern involves segregating the code into distinct phases: checks to validate inputs, effects to update state variables, and interactions with external contracts.

By adhering to this pattern, developers can ensure that each function call performs necessary validations before modifying the contract state or interacting with external contracts. This approach enhances the robustness and reliability of the codebase, mitigating potential vulnerabilities and reducing the risk of unintended behavior or exploits.

Furthermore, incorporating the checks-effects-interactions pattern promotes code clarity and maintainability by organizing the logic into well-defined stages. This architectural design facilitates debugging, auditing, and future modifications, contributing to the overall security and stability of the smart contract system.

Therefore, while the nonReentrant modifier addresses specific reentrancy concerns, combining it with the checks-effects-interactions pattern provides a comprehensive defense mechanism against a wider range of security threats and ensures the integrity and resilience of the Solidity codebase.

To improve this issue, it suggested creating a temporary variable adding existing _totalSupply and _amount and, if the check is passed, changing the actual storage _totalSupply value.

Remediation Plan

SOLVED: The Jigsaw team modified the code by first checking the condition by reading the storage and after confirmation, it is actually modified.