QA Report

[code423n4]

Handle

robee

Vulnerability details

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained (for example look at AAVE codebase).

    StakingRewards.sol, 155, require( rewardRate <= balance / rewardsDuration, "Provided reward too high" ); 

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    MasterChef.add (_depositFee)

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says. This appears in the following locations in the code base:

Deprecated safeApprove in USDMPegRecovery.sol line 79: pool3.approve(address(usdm3crv), addingLiquidity);

Deprecated safeApprove in ConvexStakingWrapper.sol line 243: lpToken.safeApprove(convexBooster, 0);

Deprecated safeApprove in ConvexStakingWrapper.sol line 241: lpToken.safeApprove(convexBooster, _amount);

Deprecated safeApprove in USDMPegRecovery.sol line 78: usdm.approve(address(usdm3crv), addingLiquidity);

Title: Require with not comprehensive message Severity: Low Risk

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: ConcurRewardPool.sol, In line 28 with Require message: !notifier
    Solidity file: USDMPegRecovery.sol, In line 69 with Require message: !oracle
    Solidity file: Shelter.sol, In line 45 with Require message: too late
    Solidity file: ConvexStakingWrapper.sol, In line 287 with Require message: too much
    Solidity file: ConvexStakingWrapper.sol, In line 258 with Require message: wait
    Solidity file: ConvexStakingWrapper.sol, In line 259 with Require message: too much

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    ConvexStakingWrapper.sol._checkpoint _account
    MasterChef.sol.safeConcurTransfer _to
    StakingRewards.sol.constructor _rewardsToken
    MasterChef.sol.withdraw _recipient
    StakingRewards.sol.setRewardsDistribution _rewardsDistribution
    VoteProxy.sol.execute _to
    EasySign.sol.modifyTeam _member
    Shelter.sol.withdraw _to
    StakingRewards.sol.constructor _stakingToken
    USDMPegRecovery.sol.constructor _kpiOracle
    ConcurRewardPool.sol.pushReward _recipient
    MasterChef.sol.removeDepositor _depositor
    ConcurRewardPool.sol.constructor _notifier
    MasterChef.sol.deposit _recipient
    MasterChef.sol.add _token
    USDMPegRecovery.sol.removeGuardian _guardian
    StakingRewards.sol.constructor _rewardsDistribution
    MasterChef.sol.addDepositor _depositor
    ConvexStakingWrapper.sol._calcRewardIntegral _account
    StakingRewards.sol.earned account
    MasterChef.sol.pendingConcur _user
    USDMPegRecovery.sol.addGuardian _guardian
    StakingRewards.sol.balanceOf account
    StakingRewards.sol.recoverERC20 tokenAddress
    VoteProxy.sol.updateAuctioneer _auctioneer
    ConvexStakingWrapper.sol._getDepositedBalance _account
    ConcurRewardPool.sol.pushReward _token

Title: Not verified claimer Severity: Low Risk

If a functions gets as input a claimer param, then it should make sure the claimer address is not address(0). Otherwise it will cause to loss of the funds or access.

    ConvexStakingWrapper.sol.setRewardPool _claimContract

Title: Treasury may be address(0) Severity: Low Risk

Make sure the treasury is not address(0).

    ConvexStakingWrapper.sol.constructor _treasury
    ConvexStakingWrapper.sol.changeTreasury _treasury

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    ConvexStakingWrapper.sol, pause is missing a reentrancy modifier
    MasterChef.sol, addDepositor is missing a reentrancy modifier
    StakingRewards.sol, setRewardsDuration is missing a reentrancy modifier
    MasterChef.sol, updatePool is missing a reentrancy modifier
    StakingRewards.sol, recoverERC20 is missing a reentrancy modifier
    MasterChef.sol, removeDepositor is missing a reentrancy modifier
    ConvexStakingWrapper.sol, unpause is missing a reentrancy modifier
    ConvexStakingWrapper.sol, requestWithdraw is missing a reentrancy modifier
    MasterChef.sol, add is missing a reentrancy modifier
    StakingRewards.sol, notifyRewardAmount is missing a reentrancy modifier
    StakingRewards.sol, exit is missing a reentrancy modifier
    ConvexStakingWrapper.sol, setRewardPool is missing a reentrancy modifier
    StakingRewards.sol, setRewardsDistribution is missing a reentrancy modifier
    ConvexStakingWrapper.sol, addRewards is missing a reentrancy modifier
    MasterChef.sol, massUpdatePools is missing a reentrancy modifier
    ConvexStakingWrapper.sol, changeTreasury is missing a reentrancy modifier

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    MasterChef.sol, massUpdatePools
    VoteProxy.sol, updateAuctioneer
    MasterChef.sol, updatePool

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    VoteProxy.sol: function execute parameter _value isn't used. (execute is external)
    VoteProxy.sol: function execute parameter _to isn't used. (execute is external)
    VoteProxy.sol: function execute parameter _data isn't used. (execute is external)

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    USDMPegRecovery.withdraw
    StakingRewards.withdraw
    MasterChef.withdraw
    ConvexStakingWrapper.withdraw
    Shelter.withdraw
    ConvexStakingWrapper.requestWithdraw

[GalloDaSballo]

Title: Mult instead div in compares Interesting find but ultimately impact is limited to %c

Title: Does not validate the input fee parameter Valid

Title: safeApprove of openZeppelin is deprecated Personally disagree but valid as non-critical

Title: Require with not comprehensive message Disagree as this is preference

Title: Not verified input Agree that some of the inputs are lacking in verification

Title: Missing non reentrancy modifier They all have nonReentrant check your scripts

Title: In the following public update functions no value is returned Good idea but impact is non-critical / informational

Title: Anyone can withdraw others I have no clue what this means, these function check the msg.sender, will mark as invalid in lack of further detail.

A couple of good findings in a sea of automated noise