StakingManagerV1 Manual Review Findings

SMV-01M: Ineffectual Stake Check

Description:

The stakeChecks modifier incorrectly utilizes the msg.sender for the validation of whether the user already exists.

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}

Recommendation:

We advise the user variable to be used instead as stakeFor would allow circumventing the minimum deposit limit.

Alleviation:

The user variable is now properly utilized in the stakeChecks modifier.

SMV-02M: Inexistent Value Sanitization

Description:

The newPercentage value is not properly sanitized.

Example:

Copy
174function updateRewardFees(uint8 newPercentage) external onlyOwner{
175    database.setRewardFee(newPercentage);
176}

Recommendation:

We advise it to be done so by ensuring it is strictly less than or equal to 100.

Alleviation:

A proper require check was introduced ensuring the newPercentage value to be less-than-or-equal-to (<=) the value of 100.

SMV-03M: Potentially Undesired Functionality

Description:

The claimRewards function cannot be invoked unless unstaking has been toggled on which may be undesirable as the claimRewards function does not actually unstake the funds of the user.

Example:

Copy
106function claimRewards() external override nonReentrant unstakeChecks {
107    rebaseEngine.rebase(msg.sender);// Must execute before stake, unstake and claim
108
109    _computeAndDisburseRewards(msg.sender);
110}

Recommendation:

We advise this to be evaluated and the unstakeChecks modifier to potentially be dropped.

Alleviation:

The unstakeChecks modifier was renamed to unstakeAndClaimChecks indicating that this is indeed desired behaviour.