DualStaking Manual Review Findings

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

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

Description:

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

Impact:

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

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 proper sanitization to be imposed, preventing the staking token from being equivalent to either of the reward tokens.

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 StakingDualRewards::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 StakingDualRewards::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.

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

Type	Severity	Location
Logical Fault		DualStaking.sol:L529-L537

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
522/**
523 * @dev Prevents a contract from calling itself, directly or indirectly.
524 * Calling a `nonReentrant` function from another `nonReentrant`
525 * function is not supported. It is possible to prevent this from happening
526 * by making the `nonReentrant` function external, and make it call a
527 * `private` function that does the actual work.
528 */
529modifier nonReentrant() {
530    _guardCounter += 1;
531    uint256 localCounter = _guardCounter;
532    _;
533    require(
534        localCounter == _guardCounter,
535        "ReentrancyGuard: reentrant call"
536    );
537}

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.

DSG-03M: Inexistent Prevention of Re-Invocation

Type	Severity	Location
Logical Fault		DualStaking.sol:L1088-L1096, L1127-L1145

Description:

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

In turn, this permits the rewardAmountA and rewardAmountB meant for multiple pools to be distributed to a single pool multiple times via direct invocation of the DualStakingRewardsFactory::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
1087// call notifyRewardAmount for all staking tokens.
1088function notifyRewardAmounts() public {
1089    require(
1090        stakingTokens.length > 0,
1091        "StakingRewardsFactory::notifyRewardAmounts: called before any deploys"
1092    );
1093    for (uint256 i = 0; i < stakingTokens.length; i++) {
1094        notifyRewardAmount(stakingTokens[i]);
1095    }
1096}
1097
1098// notify reward amount for an individual staking token.
1099// this is a fallback in case the notifyRewardAmounts costs too much gas to call for all contracts
1100function notifyRewardAmount(address stakingToken) public {
1101    require(
1102        block.timestamp >= stakingRewardsGenesis,
1103        "StakingRewardsFactory::notifyRewardAmount: not ready"
1104    );
1105
1106    StakingRewardsInfo storage info = stakingRewardsInfoByStakingToken[
1107        stakingToken
1108    ];
1109    require(
1110        info.stakingRewards != address(0),
1111        "StakingRewardsFactory::notifyRewardAmount: not deployed"
1112    );
1113
1114    if (
1115        info.rewardAmountA > 0 &&
1116        info.rewardAmountB > 0 &&
1117        info.duration > 0
1118    ) {
1119        uint256 rewardAmountA = info.rewardAmountA;
1120        uint256 rewardAmountB = info.rewardAmountB;
1121        uint256 duration = info.duration;
1122        address stakingPool = info.stakingRewards;
1123        info.rewardAmountA = 0;
1124        info.rewardAmountB = 0;
1125        info.duration = 0;
1126
1127        require(
1128            IERC20(info.rewardsTokenA).transfer(
1129                stakingPool,
1130                rewardAmountA
1131            ),
1132            "StakingRewardsFactory::notifyRewardAmount: transfer failed"
1133        );
1134        require(
1135            IERC20(info.rewardsTokenB).transfer(
1136                stakingPool,
1137                rewardAmountB
1138            ),
1139            "StakingRewardsFactory::notifyRewardAmount: transfer failed"
1140        );
1141        StakingDualRewards(stakingPool).notifyRewardAmount(
1142            rewardAmountA,
1143            rewardAmountB,
1144            duration
1145        );
1146        emit NotifyPool(
1147            stakingPool,
1148            stakingToken,
1149            rewardAmountA,
1150            rewardAmountB,
1151            duration
1152        );
1153    }
1154}

Recommendation:

We advise the code to prevent re-invocation of the same pool, and to potentially prevent direct invocation of the DualStakingRewardsFactory::notifyRewardAmount function, ensuring that rewards meant for multiple pools in custody of the DualStakingRewardsFactory 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.