StakingManagerV1 Code Style Findings

SMV-01C: Code Inconsistency

Description:

The code is not consistent in the way it validates a user's existence.

Example:

Copy
33modifier stakeChecks(address user, uint amount) {
34    require(STAKING_ON, "Staking is not allowed right now");
35    require(amount > 0, "Invalid amount to stake");
36    if(!database.userExists(msg.sender)) { // If new user then apply the minimum stake amount rule
37        require(amount >= database.getMinimumStakeAmount(), "Please try a higher value to stake");
38    }
39    _;
40}
41
42modifier unstakeChecks() {
43    require(UNSTAKING_ON, "Reward claiming is not allowed right now");
44    require(database.getUserStake(msg.sender).exist, "Invalid User");
45    require(block.timestamp - database.getUserStake(msg.sender).createdOn >= database.getMinimumStakePeriod(), "Minimum staking period not completed");
46
47    _;
48}

Recommendation:

We advise the validation check to be streamlined across the contract.

Alleviation:

The validation method was streamlined to the userExists function of the database.

SMV-02C: Inexistent Visibility Specifiers

Description:

The linked variables contain no visibility specifier explicitly set.

Example:

Copy
16IERC20 stakingToken;
17StakingDatabase database;
18IRebaseEngine rebaseEngine;
19INetworkDemand networkDemand;

Recommendation:

We advise one to be set for each variable as currently the compiler assigns a visibility specifier automatically which can cause compilation discrepancies should the default visibility change.

Alleviation:

Visibility specifiers were introduced for the first 4 variables, however, the bool variables remain without a visibility specifier.

SMV-03C: Redundant Logic Paths

Description:

The _unstake function will always yield true during its successful execution, ensuring that the if statement it is used in will always be executed.

Example:

Copy
229function _unstake(address user, uint unStakingAmount) internal returns(bool){
230
231    Stake memory userStake = database.getUserStake(user);
232
233    database.subPoolTotalStaked(unStakingAmount);
234    database.updateStakeAmount(user, userStake.amount - unStakingAmount);
235
236    return true;
237}

Recommendation:

We advise the _unstake function to no longer yield a bool and the if clause to be dropped entirely as it is ineffectual.

Alleviation:

Our recommendation was followed to the letter whereby the if clause was dropped and the _unstake function now no longer returns a bool value.

SMV-04C: Variable Mutability Specifier

Description:

The stakingToken variable is assigned to only once during the contract's constructor.

Example:

Copy
50constructor(IERC20 _stakingToken) {
51    stakingToken = _stakingToken;
52}

Recommendation:

We advise it to be set as immutable greatly optimizing the contract's gas cost.

Alleviation:

The immutable attribute was properly introduced to the linked variable.