DualStaking Static Analysis Findings

DSG-01S: Inexistent Event Emission

Type	Severity	Location
Language Specific		DualStaking.sol:L894-L896

Description:

The linked function adjusts a sensitive contract variable yet does not emit an event for it.

Example:

Copy
894function toggleWithdrawBeforeEnd() external onlyOwner {
895    isLocked = !isLocked;
896}

Recommendation:

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

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The WithdrawLockToggled event was introduced to the codebase and is correspondingly emitted in the StakingDualRewards::toggleWithdrawBeforeEnd function, addressing this exhibit in full.

DSG-02S: Inexistent Sanitization of Input Addresses

Type	Severity	Location
Input Sanitization		DualStaking.sol:L710-L727

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
710constructor(
711    address _owner,
712    address _dualRewardsDistribution,
713    address _rewardsTokenA,
714    address _rewardsTokenB,
715    address _stakingToken,
716    bool _isLocked
717) public Owned(_owner) {
718    require(
719        _rewardsTokenA != _rewardsTokenB,
720        "rewards tokens should be different"
721    );
722    rewardsTokenA = IERC20(_rewardsTokenA);
723    rewardsTokenB = IERC20(_rewardsTokenB);
724    stakingToken = IERC20(_stakingToken);
725    dualRewardsDistribution = _dualRewardsDistribution;
726    isLocked = _isLocked;
727}

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 StakingDualRewards::constructor function are adequately sanitized as non-zero in the latest in-scope revision of the codebase, addressing this exhibit.

DSG-03S: Multiple Top-Level Declarations

Type	Severity	Location
Code Style		DualStaking.sol:L70, L108, L205, L293, L348, L382, L512, L540, L568, L611, L632, L677, L942

Description:

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

Example:

Copy
70library Math {
71    /**
72     * @dev Returns the largest of two numbers.
73     */
74    function max(uint256 a, uint256 b) internal pure returns (uint256) {
75        return a >= b ? a : b;
76    }
77
78    /**
79     * @dev Returns the smallest of two numbers.
80     */
81    function min(uint256 a, uint256 b) internal pure returns (uint256) {
82        return a < b ? a : b;
83    }
84
85    /**
86     * @dev Returns the average of two numbers. The result is rounded towards
87     * zero.
88     */
89    function average(uint256 a, uint256 b) internal pure returns (uint256) {
90        // (a + b) / 2 can overflow, so we distribute
91        return (a / 2) + (b / 2) + (((a % 2) + (b % 2)) / 2);
92    }
93}
94
95/**
96 * @dev Wrappers over Solidity's arithmetic operations with added overflow
97 * checks.
98 *
99 * Arithmetic operations in Solidity wrap on overflow. This can easily result
100 * in bugs, because programmers usually assume that an overflow raises an
101 * error, which is the standard behavior in high level programming languages.
102 * `SafeMath` restores this intuition by reverting the transaction when an
103 * operation overflows.
104 *
105 * Using this library instead of the unchecked operations eliminates an entire
106 * class of bugs, so it's recommended to use it always.
107 */
108library SafeMath {

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.

DSG-04S: Improper Invocations of EIP-20 transfer

Type	Severity	Location
Standard Conformity		DualStaking.sol:L1127-L1133, L1134-L1140, L1160

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
1127require(
1128    IERC20(info.rewardsTokenA).transfer(
1129        stakingPool,
1130        rewardAmountA
1131    ),
1132    "StakingRewardsFactory::notifyRewardAmount: transfer failed"
1133);

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.