SingleStaking Manual Review Findings

SSG-01M: Inexistent Prevention of Equivalent Reward & Staking

Type	Severity	Location
Input Sanitization		SingleStaking.sol:L639-L651

Description:

The StakingSingleRewards::constructor does not prevent the stakingToken to be set as a reward token which we consider invalid.

Impact:

The code of the StakingSingleRewards contract is not compatible with a staking token that is equivalent to one of its rewards, a configuration presently permitted.

Example:

Copy
639constructor(
640    address _rewardsDistribution,
641    address _rewardsToken,
642    address _stakingToken,
643    bool _isLocked,
644    address _manager
645) public {
646    rewardsToken = IERC20(_rewardsToken);
647    stakingToken = IERC20(_stakingToken);
648    rewardsDistribution = _rewardsDistribution;
649    isLocked = _isLocked;
650    manager = _manager;
651}

Recommendation:

We advise proper sanitization to be imposed, preventing the staking token from being equivalent to the reward token.

Alleviation (dc34c6a066):

After re-evaluating the contract in light of the specification provided to us by the Steer Protocol team, we identified two issues that arise from a reward token being equivalent to the staking token.

The first issue that arises is the fact that the StakingSingleRewards::notifyRewardAmount function will not impose its balance-related security mechanism correctly. In detail, it permits rewards to be distributed even though they have not been transmitted to the contract, effectively distributing the underlying balances of users as rewards.

The second issue resulting from such a configuration lies in the StakingSingleRewards::recoverERC20 function and specifically the rescue mechanism. In contrast to the code's expected behaviour, the rescue mechanism will not permit rewarded tokens to be rescued due to the staking token related security mechanism.

To alleviate both issues, we advise the former to subtract the _totalSupply from the IERC20::balanceOf measurement and the latter to subtract the _totalSupply from the permitted amount to be rescued if the tokenAddress argument is equivalent to the stakingToken.

Alleviation (3607cb04d9):

The recommended optimizations have been applied as advised to the relevant functions, rendering this exhibit fully alleviated.

SSG-02M: Non-Standard Re-Entrancy Protection

Type	Severity	Location
Logical Fault		SingleStaking.sol:L562-L568

Description:

The referenced ReentrancyGuard::nonReentrant implementation is non-standard as it will utilize a continuously incremented _guardCounter instead of configuring the counter from 1 to 2 and vice versa. This permits the inner-most re-entrant call to successfully execute, which is an incorrect approach.

Impact:

The present re-entrancy guard will fail after the execution of a function's body, and will not fail at the innermost call, both of which are ill-advised security traits.

Example:

Copy
554/**
555 * @dev Prevents a contract from calling itself, directly or indirectly.
556 * Calling a `nonReentrant` function from another `nonReentrant`
557 * function is not supported. It is possible to prevent this from happening
558 * by making the `nonReentrant` function external, and make it call a
559 * `private` function that does the actual work.
560 */
561modifier nonReentrant() {
562    _guardCounter += 1;
563    uint256 localCounter = _guardCounter;
564    _;
565    require(
566        localCounter == _guardCounter,
567        "ReentrancyGuard: reentrant call"
568    );
569}

Recommendation:

We advise the code to mimic the latest re-entrancy guard pattern by OpenZeppelin, toggling the value of _guardCounter between 1 and 2 thus ensuring that the contract is in a not-entered state before the execution of the modifier's function body (_;).

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The OpenZeppelin re-entrancy guard pattern has been adopted by the contract, alleviating this exhibit.

SSG-03M: Inexistent Prevention of Re-Invocation

Type	Severity	Location
Logical Fault		SingleStaking.sol:L932-L940, L944-L975

Description:

The SingleStakingRewardsFactory::notifyRewardAmounts and SingleStakingRewardsFactory::notifyRewardAmount functions can be repeatedly invoked.

In turn, this permits the rewardAmount meant for multiple pools to be distributed to a single pool multiple times via direct invocation of the SingleStakingRewardsFactory::notifyRewardAmount function.

Impact:

As the functions do not impose any access control, a malicious party can stake in a single pool and repeatedly invoke the function to skew the reward distribution towards the pool they staked thus profiting from the attack.

Example:

Copy
931// call notifyRewardAmount for all staking tokens.
932function notifyRewardAmounts() public {
933    require(
934        stakingTokens.length > 0,
935        "StakingRewardsFactory::notifyRewardAmounts: called before any deploys"
936    );
937    for (uint256 i = 0; i < stakingTokens.length; i++) {
938        notifyRewardAmount(stakingTokens[i]);
939    }
940}
941
942// notify reward amount for an individual staking token.
943// this is a fallback in case the notifyRewardAmounts costs too much gas to call for all contracts
944function notifyRewardAmount(address stakingToken) public {
945    require(
946        block.timestamp >= stakingRewardsGenesis,
947        "StakingRewardsFactory::notifyRewardAmount: not ready"
948    );
949
950    StakingRewardsInfo storage info = stakingRewardsInfoByStakingToken[
951        stakingToken
952    ];
953    address stakingPool = info.stakingRewards;
954    require(
955        stakingPool != address(0),
956        "StakingRewardsFactory::notifyRewardAmount: not deployed"
957    );
958
959    if (info.rewardAmount > 0 && info.duration > 0) {
960        uint256 rewardAmount = info.rewardAmount;
961        uint256 duration = info.duration;
962        info.rewardAmount = 0;
963        info.duration = 0;
964
965        require(
966            IERC20(info.rewardsToken).transfer(stakingPool, rewardAmount),
967            "StakingRewardsFactory::notifyRewardAmount: transfer failed"
968        );
969        StakingSingleRewards(stakingPool).notifyRewardAmount(
970            rewardAmount,
971            duration
972        );
973        emit NotifyPool(stakingPool, stakingToken, rewardAmount, duration);
974    }
975}

Recommendation:

We advise the code to prevent re-invocation of the same pool, and to potentially prevent direct invocation of the SingleStakingRewardsFactory::notifyRewardAmount function, ensuring that rewards meant for multiple pools in custody of the SingleStakingRewardsFactory cannot be compromised.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The Steer Protocol team evaluated this exhibit and evaluated that the exhibit is inapplicable as the entries of the StakingRewardsInfo structure are zeroed out before disbursing the rewards to the relevant staking pool, effectively disallowing re-invocation of the same stakingToken.

We analysed the Steer Protocol team's response and concluded that it is correct, effectively rendering this exhibit inapplicable.