StakingRewards Manual Review Findings

SRS-01M: Inexistent Initialization of Base Implementation

Type	Severity	Location
Language Specific		StakingRewards.sol:L16-L21

Description:

The contract does not properly initialize the base logic implementation permitting it to be taken over by a malicious party.

Impact:

While not an active security threat, it can evolve into one if any form of delegatecall capability is introduced in one of the dependencies of the contract that could cause it to invoke a selfdestruct instruction.

Example:

Copy
16contract StakingRewards is
17    IStakingRewards,
18    Initializable,
19    OwnableUpgradeable,
20    UUPSUpgradeable
21{

Recommendation:

We advise a constructor to be introduced that simply invokes the initializer modifier to ensure that the logic implementation cannot be initialized maliciously.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

A constructor was introduced that properly invokes the initializer modifier and disallows initialization of the logic implementation, alleviating this exhibit in full.

SRS-02M: Potentially Insecure Accounting System

Type	Severity	Location
Logical Fault		StakingRewards.sol:L100, L211

Description:

The accounting system of the StakingRewards contract is protected indirectly by its uninitialized variables, however, this trait is ill-advised as it can lead to complications in potential future updates of the contract.

Impact:

While it currently does not present an active security threat, the updateReward should not touch the rewards mapping of a user if they have no staked funds.

Example:

Copy
206function updateReward(uint256 _poolId, pool memory _pool) internal {
207    uint256 stakeTime;
208    if (block.timestamp > _pool.end) stakeTime = _pool.end;
209    else stakeTime = block.timestamp;
210    uint256 lastReward = (_balances[msg.sender][_poolId] *
211        (((stakeTime - lastRewarded[msg.sender][_poolId]) *
212            (_pool.rewardRate * PRECISION)) /
213            (RATE_PRECISION * SECONDS_IN_YEAR))) / PRECISION;
214    lastRewarded[msg.sender][_poolId] = stakeTime;
215    rewards[msg.sender][_poolId] += lastReward;
216}

Recommendation:

We advise the updateReward function to perform a no-op of the balance of the user is zero as it currently calculates a lastReward of 0 indirectly due to the balance being zero. Instead, if the balance of the user is zero it should simply set the lastRewarded value to the current stakeTime.

Alleviation (0ed41ccc18a72b7e559b8d79ab7ba6172362ee3b):

The code now correctly updates the rewards of a user conditionally if the user has a non-zero balance, properly alleviating this exhibit.

SRS-03M: Inexistent Validation of Pool Initialization

Type	Severity	Location
Input Sanitization		StakingRewards.sol:L70-L86

Description:

The createPool function does not impose any sanitization on the pool's configuration parameters.

Impact:

If a pool is misconfigured it will not distribute rewards properly and may not even function at all.

Example:

Copy
70function createPool(
71    address _stakingToken,
72    address _rewardToken,
73    uint256 _rewardRate,
74    uint256 _start,
75    uint256 _end
76) external onlyOwner {
77    uint256 _totalPools = totalPools;
78    Pools[_totalPools] = pool({
79        stakingToken: _stakingToken,
80        rewardToken: _rewardToken,
81        rewardRate: _rewardRate * 100,
82        start: _start,
83        end: _end
84    });
85    totalPools = _totalPools + 1;
86}

Recommendation:

We advise the _start value to be validated as less-than (<) the _end and the _stakingToken to be different than the _rewardToken, ensuring that pools created via this way behave properly.

Alleviation (200f275c40):

While the start < end comparison has been adequately introduced, the staking and reward tokens are not compared between them which can cause complications in the rewarding mechanism of the contract.

Alleviation (0ed41ccc18):

Offering rewards in the same token as the one being staked will cause complications in the balance measurements, the transfers performed as well as the availability of funds in the contract for paying out rewards (as an example, an over-abundance of rewards would cause stakes to be impossible to withdraw). To avoid such scenarios, we strongly advise against same-token pools and we instead urge the Steer Protocol to evaluate alternatives, such as wrapper tokens that are staked which are distinct from the reward token.

Alleviation (878a807a67):

A require check has been properly introduced ensuring that the _stakingToken is different from the _rewardToken thus alleviating this exhibit in full.

SRS-04M: Improper Overwrite of Pending Rewards

Type	Severity	Location
Logical Fault		StakingRewards.sol:L143

Description:

The else branch of the claimReward function will override any pre-existing pendingRewards with new ones that do not account for historically not distributed rewards.

Impact:

Currently, if the pool is unable to pay out the rewards for a long period of time any consecutive invocations of claimReward will overwrite the previously stored pendingRewards thus causing rewards for a particular user to be permanently lost.

Example:

Copy
128function claimReward(uint256 _poolId, pool memory _pool) internal {
129    updateReward(_poolId, _pool);
130    uint256 reward = rewards[msg.sender][_poolId];
131    rewards[msg.sender][_poolId] = 0;
132    _balances[msg.sender][_poolId] = 0;
133    lastRewarded[msg.sender][_poolId] = 0;
134    uint256 totalRewards = totalRewardsPerPool[_poolId];
135    if (totalRewards >= reward) {
136        totalRewardsPerPool[_poolId] = totalRewards - reward;
137        emit RewardPaid(msg.sender, _poolId, reward);
138        IERC20Upgradeable(_pool.stakingToken).safeTransfer(
139            msg.sender,
140            reward
141        );
142    } else {
143        pendingRewards[msg.sender][_poolId] = reward - totalRewards;
144        totalRewardsPerPool[_poolId] = 0;
145        emit RewardPaid(msg.sender, _poolId, totalRewards);
146        IERC20Upgradeable(_pool.stakingToken).safeTransfer(
147            msg.sender,
148            totalRewards
149        );
150    }
151}

Recommendation:

We advise the pendingRewards variable to become cumulative by adding to it instead of overwriting as otherwise users will lose their pending rewards in a sub-optimal system state.

Alleviation (200f275c40):

The unstake function introduced a require check ensuring that it cannot be invoked with a zero-value amount being unstaked, protecting against repetitive invocations of the claimReward function which would overwrite rewards. However, the overwrite operation can still occur in case unstake is invoked, a stake / stakeFor operation is performed, and another unstake occurs. As such, we advise either the pendingRewards variable to become cumulative or the claimReward function to prevent invocation if pendingRewards is non-zero, forcing the user to claim their pending rewards prior to requesting more.

Alleviation (0ed41ccc18):

After discussions with the Steer Protocol team, we evaluated that the scenario described above cannot materialize in the current state of the code as an unstake operation prior to the staking period's conclusion would cause no rewards to be stored and thus no rewards to be overwritten in a consequent unstake operation. In any case, the pending rewards becoming cumulative is a change with minimal impact to the code and significant value in minimizing accounting mistakes. As such, we consider this exhibit alleviated albeit still urge the Steer Protocol team to render rewards cumulative.