SingleStaking Static Analysis Findings

SSG-01S: Illegible Numeric Value Representation

Type	Severity	Location
Code Style		SingleStaking.sol:L668

Description:

The linked representation of a numeric literal is sub-optimally represented decreasing the legibility of the codebase.

Example:

Copy
668return rewardRate * 86400;

Recommendation:

To properly illustrate the value's purpose, we advise the following guidelines to be followed. For values meant to depict fractions with a base of 1e18, we advise fractions to be utilized directly (i.e. 1e17 becomes 0.1e18) as they are supported. For values meant to represent a percentage base, we advise each value to utilize the underscore (_) separator to discern the percentage decimal (i.e. 10000 becomes 100_00, 300 becomes 3_00 and so on). Finally, for large numeric values we simply advise the underscore character to be utilized again to represent them (i.e. 1000000 becomes 1_000_000).

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The Steer Protocol team evaluated this exhibit and opted to selectively apply the numeric representation style where they deemed appropriate.

As such, we consider this exhibit alleviated based on the preferences of the Steer Protocol team.

SSG-02S: Inexistent Event Emissions

Type	Severity	Location
Language Specific		SingleStaking.sol:L777-L780, L782-L785

Description:

The linked functions adjust sensitive contract variables yet do not emit an event for it.

Example:

Copy
777function toggleWithdrawBeforeEnd() external {
778    require(msg.sender == manager, "Unauthorized");
779    isLocked = !isLocked;
780}

Recommendation:

We advise an event to be declared and correspondingly emitted for each function to ensure off-chain processes can properly react to this system adjustment.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The WithdrawLockToggled, and PauseToggled events were introduced to the codebase and are correspondingly emitted in the StakingSingleRewards::toggleWithdrawBeforeEnd, and StakingSingleRewards::togglePause functions respectively, addressing this exhibit in full.

SSG-03S: Inexistent Sanitization of Input Addresses

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

Description:

The linked function(s) accept address arguments yet do not properly sanitize them.

Impact:

The presence of zero-value addresses, especially in constructor implementations, can cause the contract to be permanently inoperable. These checks are advised as zero-value inputs are a common side-effect of off-chain software related bugs.

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 some basic sanitization to be put in place by ensuring that each address specified is non-zero.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

All input arguments of the StakingSingleRewards::constructor function are adequately sanitized as non-zero in the latest in-scope revision of the codebase, addressing this exhibit.

SSG-04S: Multiple Top-Level Declarations

Type	Severity	Location
Code Style		SingleStaking.sol:L23, L117, L201, L297, L325, L380, L414, L544, L573, L596, L610, L819

Description:

The referenced file contains multiple top-level declarations that decrease the legibility of the codebase.

Example:

Copy
23interface IERC20 {
24    /**
25     * @dev Returns the amount of tokens in existence.
26     */
27    function totalSupply() external view returns (uint256);
28
29    /**
30     * @dev Returns the amount of tokens owned by `account`.
31     */
32    function balanceOf(address account) external view returns (uint256);
33
34    /**
35     * @dev Moves `amount` tokens from the caller's account to `recipient`.
36     *
37     * Returns a boolean value indicating whether the operation succeeded.
38     *
39     * Emits a `Transfer` event.
40     */
41    function transfer(
42        address recipient,
43        uint256 amount
44    ) external returns (bool);
45
46    /**
47     * @dev Returns the remaining number of tokens that `spender` will be
48     * allowed to spend on behalf of `owner` through `transferFrom`. This is
49     * zero by default.
50     *
51     * This value changes when `approve` or `transferFrom` are called.
52     */
53    function allowance(
54        address owner,
55        address spender
56    ) external view returns (uint256);
57
58    /**
59     * @dev Sets `amount` as the allowance of `spender` over the caller's tokens.
60     *
61     * Returns a boolean value indicating whether the operation succeeded.
62     *
63     * > Beware that changing an allowance with this method brings the risk
64     * that someone may use both the old and the new allowance by unfortunate
65     * transaction ordering. One possible solution to mitigate this race
66     * condition is to first reduce the spender's allowance to 0 and set the
67     * desired value afterwards:
68     * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
69     *
70     * Emits an `Approval` event.
71     */
72    function approve(address spender, uint256 amount) external returns (bool);
73
74    /**
75     * @dev Moves `amount` tokens from `sender` to `recipient` using the
76     * allowance mechanism. `amount` is then deducted from the caller's
77     * allowance.
78     *
79     * Returns a boolean value indicating whether the operation succeeded.
80     *
81     * Emits a `Transfer` event.
82     */
83    function transferFrom(
84        address sender,
85        address recipient,
86        uint256 amount
87    ) external returns (bool);
88
89    /**
90     * @dev Emitted when `value` tokens are moved from one account (`from`) to
91     * another (`to`).
92     *
93     * Note that `value` may be zero.
94     */
95    event Transfer(address indexed from, address indexed to, uint256 value);
96
97    /**
98     * @dev Emitted when the allowance of a `spender` for an `owner` is set by
99     * a call to `approve`. `value` is the new allowance.
100     */
101    event Approval(
102        address indexed owner,
103        address indexed spender,
104        uint256 value
105    );
106}
107
108/**
109 * @dev Contract module which provides a basic access control mechanism, where
110 * there is an account (an owner) that can be granted exclusive access to
111 * specific functions.
112 *
113 * This module is used through inheritance. It will make available the modifier
114 * `onlyOwner`, which can be aplied to your functions to restrict their use to
115 * the owner.
116 */
117contract Ownable {

Recommendation:

We advise all highlighted top-level declarations to be split into their respective code files, avoiding unnecessary imports as well as increasing the legibility of the codebase.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The Steer Protocol team evaluated this exhibit and due to time constraints have opted to retain a flat file structure for the time being.

SSG-05S: Improper Invocations of EIP-20 transfer

Type	Severity	Location
Standard Conformity		SingleStaking.sol:L965-L968, L981

Description:

The linked statement do not properly validate the returned bool of the EIP-20 standard transfer function. As the standard dictates, callers must not assume that false is never returned.

Impact:

If the code mandates that the returned bool is true, this will cause incompatibility with tokens such as USDT / Tether as no such bool is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.

Example:

Copy
965require(
966    IERC20(info.rewardsToken).transfer(stakingPool, rewardAmount),
967    "StakingRewardsFactory::notifyRewardAmount: transfer failed"
968);

Recommendation:

Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists in each instance.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The SafeERC20 library of the OpenZeppelin dependency is now properly imported to the codebase and its SafeERC20::safeTransfer function is correctly invoked in place of all potentially unhandled EIP-20 ERC20::transfer invocations, addressing this exhibit.