6.1 The stake function doesn't check if the node is allowListed
The IDOSNodeStaking.stake(...) function doesn't check if the input parameter node is part of the allowlistedNodes set.
function stake(address user, address node, uint256 amount) external nonReentrant whenNotPaused {
//@audit--issue lack of checks that the node is actually allowlisted
require(node != address(0), ZeroAddressNode());
require(!slashedNodes.contains(node), NodeIsSlashed(node));
require(amount > 0, AmountNotPositive(amount));
require(block.timestamp >= startTime, NotStarted());
if (user == address(0)) user = msg.sender;
//..
//..
}
This allows users to pick any arbitrary address as the node to stake with. A user can even pick their own address as the node. This would allow a node to be both a staker and node at the same time. The problem is that these stakers will accrue rewards, just like the ones who staked on the allowlistedNodes would and this enables malicious users to steal the staking rewards from honest stakers and nodes.
As a side note, the unstake(...) and slash(...) functions don't implement this check either.
Recommendation
Consider adding a check that the node input parameter is part of the allowlistedNodes list.
6.2 The withdrawableReward function uses incorrect reward values for epochs
The withdrawableReward(...) function in the IDOSNodeStaking contract calculates the pending rewards for a user by iterating through the epochs that have passed since the user's last checkpoint. The protocol starts from a default epoch reward value (stored at key 0) and allows the owner to change the epoch reward via setEpochReward. Each call to setEpochReward at epoch X is intended to define a new reward value that applies from epoch X onward until changed again (i.e. rewards are _sticky_ across epochs).
The issue lies in how the epochReward variable is initialized relative to the user's checkpoint. At the beginning of withdrawableReward(...), the epochReward variable is always set to the default reward at index 0, regardless of any changes that may have occurred before the user's last checkpoint. The function then iterates from checkpoint.epoch up to currentEpoch(), and only updates epochReward if there is an entry in epochRewardChanges at the _exact_ epoch i:
function withdrawableReward(address user)
public
view
returns (uint256 withdrawableAmount, uint256 rewardAcc, uint256 userStakeAcc, uint256 totalStakeAcc)
{
// ...
//@audit rewards are set to the default value
uint256 epochReward = epochRewardChanges.get(0);
for (uint48 i = checkpoint.epoch; i < currentEpoch(); i++) {
//@audit if rewards were not updated exactly at epoch `i` the protocol
//uses the default value, which may be incorrect
(bool exists, uint256 rewardAtEpoch) = epochRewardChanges.tryGet(i);
if (exists) epochReward = rewardAtEpoch;
// ...
if (totalStakeAcc == 0) continue;
rewardAcc += (userStakeAcc * epochReward) / totalStakeAcc;
}
//..
}
Whenever the epoch reward has been changed at some epoch X and a user’s checkpoint is at a later epoch checkpoint epoch > X, the rewards for epochs >= user's checkpoint are computed using the default reward instead of the last changed reward. This will lead to incorrect rewards calculations for stakers.
Recommendation
Consider revisiting the loop logic to ensure correct rewards calculations.
6.3 Unbounded loop in withdrawUnstaked(...) results in potential Denial of Service
The unstake(...) function allows users to initiate the withdrawal process by appending an Unstake struct to the unstakesByUser array. After the UNSTAKE_DELAY period, users call the withdrawUnstaked(...) function to claim their tokens.
However, the withdrawUnstaked(...) function iterates over the entire unstakesByUser array in a single transaction to find eligible withdrawals.
function withdrawUnstaked()
external nonReentrant whenNotPaused
returns (uint256 withdrawableAmount)
{
withdrawableAmount;
// @audit The loop iterates over the entire array which can grow indefinitely
for (uint i; i < unstakesByUser[msg.sender].length; i++)
if (unstakesByUser[msg.sender][i].timestamp < uint48(block.timestamp) - UNSTAKE_DELAY) {
withdrawableAmount += unstakesByUser[msg.sender][i].amount;
// @audit-issue 'delete' zeroes the element but does not reduce array length
delete unstakesByUser[msg.sender][i];
}
// ...
}
This logic presents two significant risks that can cause the transaction to exceed the block gas limit, leading to a Denial of Service (DoS) where funds become permanently locked:
- Inefficient Array Management: The code uses
delete to clear processed unstakes. In Solidity, delete only resets the value to default (zero) but does not remove the index or decrease the array length. Consequently, the array grows indefinitely, forcing the loop to process an increasing number of "ghost" entries from past withdrawals; - Accumulation of Pending Unstakes: If a user performs many unstake operations without calling
withdrawUnstaked(...) for a long period, the array will contain a large number of valid, pending entries. Attempting to process all of them in a single loop may consume more gas than the block limit allows;
Recommendation
Consider refactoring the withdrawal logic to prevent unbounded iteration and support batch processing. Two potential solutions include:
- Start Index and Batching: Implement a pointer or start index variable to track the head of the queue (the first unprocessed withdrawal). Combine this with a parameter that allows the user to specify the number of items to process in one transaction;
- Swap-and-Pop and Batching: Replace the
delete operation with a "swap and pop" mechanism. This involves moving the last element of the array to the position of the withdrawn element and then popping the array to decrease its length. This should also be combined with a user-provided parameter to limit the number of withdrawals processed per transaction;
Resolution
We ran the following test: after 700 noisy epochs (with 100 other users staking, unstaking, etc), we had a user stake every day and unstake every other day for another 700 quiet epochs; last unstake cost 200k gas.
6.4 Unbounded loop in withdrawableReward(...) leads to Denial of Service for new and inactive stakers
The stake(...), unstake(...) and withdrawReward(...) functions utilize the createEpochCheckpoint(...) function to update the user's state. This internal logic relies on withdrawableReward(...) to calculate the pending rewards accumulated since the user's last interaction.
However, the withdrawableReward(...) function calculates rewards by iterating from the user's last recorded epoch ( checkpoint.epoch) up to the currentEpoch(). For a new user who has never interacted with the contract, the default value of checkpoint.epoch is 0.
function withdrawableReward(address user)
public view
returns (uint256 withdrawableAmount, uint256 rewardAcc, uint256 userStakeAcc, uint256 totalStakeAcc)
{
EpochCheckpoint memory checkpoint = epochCheckpointByUser[user];
// ...
// @audit If the user is new, checkpoint.epoch is 0.
// @audit If the protocol has been running for a long time, this loop becomes extremely large.
for (uint48 i = checkpoint.epoch; i < currentEpoch(); i++) {
// ...
}
// ...
}
This creates a Denial of Service (DoS) vector in two scenarios:
- New Stakers: If the protocol has been active for a long time (a large number of epochs have passed), a new user trying to
stake(...) will trigger the loop from epoch 0 to the current epoch. If the gas cost of this iteration exceeds the block gas limit, the user effectively cannot stake; - Inactive Stakers: Existing users who have not interacted with the contract for a long period will face the same issue when trying to
unstake(...) or claim rewards;
Recommendation
Consider implementing the following changes to mitigate the DoS risk:
- Pagination: Update the logic to allow processing epochs in batches. This allows users to update their checkpoints incrementally across multiple transactions rather than forcing the calculation of all missing epochs in a single transaction;
- Optimize for New Users: Check if a user is a new staker (e.g., has a stake of 0 and no history). If so, set their
checkpoint.epoch directly to currentEpoch() to avoid iterating through history where they had no stake; - Distinguish Epoch 0: To safely distinguish between a "new user" (default 0) and a user who genuinely staked at epoch 0, ensure the first valid epoch of the protocol is
1 (or higher) by adjusting the startTime in the constructor;
Note regarding economic viability: While pagination solves the hard DoS limits, if the gas cost to process a single epoch iteration exceeds the rewards earned in that epoch, the function becomes economically unviable to execute. However, given that the contract is deployed on Arbitrum One which is expected to have low gas fees, this specific economic edge case is considered unlikely to happen.
Resolution
If a user stakes after 1400 epochs, the transaction still has enough gas to go through. Furthermore, the contract will not be used for more than a couple of months before being replaced, so it will be replaced before this issue can occur.
6.5 Users can evade slashing penalties by front-running the slash(...) transaction
The IDOSNodeStaking contract allows users to delegate their tokens to nodes via the stake(...) function. If a node acts maliciously, the contract owner can call the slash(...) function to penalize the node. This action is intended to freeze the stakes associated with that node, preventing withdrawals. Users can normally retrieve their funds by calling unstake(...), waiting for the UNSTAKE_DELAY, and then calling withdrawUnstaked(...).
The vulnerability lies in the fact that unstake(...) immediately reduces the staked balance associated with the node in the stakeByNode and stakeByNodeByUser mappings. If a user anticipates that a node is about to be slashed (e.g., through off-chain monitoring or governance announcements), they can call unstake(...) before the slash(...) transaction is executed.
Once the user calls unstake(...), the funds are moved to a pending state in the unstakesByUser array, effectively dissociating them from the node. When slash(...) is subsequently executed, it only locks the funds currently staked on the node. The withdrawUnstaked(...) function does not verify if the node associated with the pending withdrawal was slashed. Consequently, the user can withdraw their funds after the delay, successfully bypassing the slashing penalty.
Although the contract is deployed on Arbitrum One, which mitigates standard mempool front-running, this issue remains relevant if the slashing decision is predictable or communicated off-chain prior to on-chain execution.
function unstake(address node, uint256 amount)
external nonReentrant whenNotPaused
{
// ...
// @audit Checks if node is currently slashed, but doesn't prevent future slashing evasion
require(!slashedNodes.contains(node), NodeIsSlashed(node));
// ...
// @audit-issue User stake is immediately decremented from the node mapping
stakeByNodeByUser[msg.sender][node] -= amount;
// ...
// @audit-issue Node total stake is immediately reduced
uint256 newNodeStake = getNodeStake(node) - amount;
if (newNodeStake > 0) {
stakeByNode.set(node, newNodeStake);
} else {
stakeByNode.remove(node);
}
// ...
// @audit The pending unstake does not track which node these funds came from
unstakesByUser[msg.sender].push(Unstake(amount, uint48(block.timestamp)));
// ...
}
Recommendation
Consider updating the Unstake struct to store the address of the node being unstaked. In the withdrawUnstaked(...) function, introduce logic to verify if the corresponding node has been slashed.
To ensure fairness, distinguish between long-standing unstake requests and those made immediately prior to a slashing event. Consider checking if the node was slashed within a specific window relative to the unstake timestamp. If the slash happened shortly after the unstake, the withdrawal should be blocked or penalized.
Crucially, if a user's withdrawal is blocked due to slashing, ensure the system correctly accounts for these funds so they are recoverable by the protocol. Since unstake(...) reduces the stakeByNode mapping, these funds would not normally be captured by the withdrawSlashedStakes(...) function (which iterates over current node stakes). The logic must essentially revert the accounting reductions made during unstake(...) or explicitly route these blocked funds to the slashed pool to ensure they can be withdrawn by the owner.
Resolution
This applies only to “off-chain front-running” since Arbitrum One has no public mempool. And validators won’t risk slashing by announcing slashable offenses publicly. We don’t really expect slashing with the small and trusted validator set we’ll have for the foreseeable future (note again this contract is unlikely to live for more than a few months).
6.6 Users can manipulate reward distribution by staking at the end of an epoch
The IDOSNodeStaking contract incentivizes users to stake tokens delegated to nodes by distributing rewards over fixed time periods called epochs. The duration of an epoch is defined by EPOCH_LENGTH, which is set to 1 day. Users can stake tokens using the stake(...) function and claim their accumulated rewards via withdrawReward(...).
The issue arises from the way rewards are calculated relative to the time of staking. When a user calls stake(...), their stake is recorded for the current epoch. Subsequently, the withdrawableReward(...) function calculates rewards by iterating through completed epochs. It uses the total stake recorded for a specific epoch to determine the user's share of the rewards. However, the calculation does not account for the duration the tokens were actually staked within that epoch.
This allows a malicious user to deposit a large amount of tokens just before the epoch ends (e.g., in the last second). As soon as the next epoch begins, the protocol considers their stake to have been active for the entire duration of the previous epoch. Consequently, the attacker receives the full reward for that epoch despite their funds being exposed to the protocol (and potential slashing risks) for only a brief moment. This behavior dilutes the rewards of honest stakers who risk their capital for the full duration.
The stake(...) function updates the mapping for the current epoch immediately:
function stake(address user, address node, uint256 amount)
external nonReentrant whenNotPaused
{
// ...
stakeByNodeByUser[user][node] += amount;
// @audit The stake is added to the current epoch's balance immediately
stakeByUserByEpoch[currentEpoch()][user] += amount;
stakedByEpoch[currentEpoch()] += amount;
// ...
}
The withdrawableReward(...) function calculates the reward based on that balance, treating it as valid for the whole epoch:
function withdrawableReward(address user)
public view
returns (uint256 withdrawableAmount, uint256 rewardAcc, uint256 userStakeAcc, uint256 totalStakeAcc)
{
// ...
for (uint48 i = checkpoint.epoch; i < currentEpoch(); i++) {
// ...
// @audit The user's stake is added to the accumulator for the entire epoch `i`
userStakeAcc += stakeByUserByEpoch[i][user];
userStakeAcc -= unstakeByUserByEpoch[i][user];
totalStakeAcc += stakedByEpoch[i];
// ...
// @audit Rewards are distributed based on the final stake of the epoch
rewardAcc += (userStakeAcc * epochReward) / totalStakeAcc;
}
// ...
}
Recommendation
Consider modifying the staking logic so that deposits made during the current epoch only become eligible for rewards starting from the subsequent epoch. This ensures that a user's funds must be staked for at least one full epoch cycle before earning rewards, aligning the reward incentive with the risk exposure.
Resolution
This was a deliberate trade-off. We eschewed the complexity of implementing a more complex AMM/staking pattern (e.g. Synthetix, Uniswap) by using short 1-day epochs combined with a 14-day unbonding period.
