QA Report

[code423n4]

Should use modifier instead of function call for role based access control

For example

    /// @dev Change Delegation to another address
    function manualSetDelegate(address delegate) external {
        _onlyGovernance();
        // Set delegate is enough as it will clear previous delegate automatically
        LOCKER.delegate(delegate);
    }

    ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
    function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external {
        _onlyGovernance();
        withdrawalSafetyCheck = newWithdrawalSafetyCheck;
    }

    ///@dev Should we processExpiredLocks during reinvest?
    function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external {
        _onlyGovernance();
        processLocksOnReinvest = newProcessLocksOnReinvest;
    }

     ///@dev Change the contract that handles bribes
    function setBribesProcessor(IBribesProcessor newBribesProcessor) external {
        _onlyGovernance();
        bribesProcessor = newBribesProcessor;
    }

Should be converted to

    modifier onlyGovernance() {
        _onlyGovernance();
    }

    /// @dev Change Delegation to another address
    function manualSetDelegate(address delegate) external onlyGovernance {
        // Set delegate is enough as it will clear previous delegate automatically
        LOCKER.delegate(delegate);
    }

    ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
    function setWithdrawalSafetyCheck(bool newWithdrawalSafetyCheck) external onlyGovernance {
        withdrawalSafetyCheck = newWithdrawalSafetyCheck;
    }

    ///@dev Should we processExpiredLocks during reinvest?
    function setProcessLocksOnReinvest(bool newProcessLocksOnReinvest) external onlyGovernance {
        processLocksOnReinvest = newProcessLocksOnReinvest;
    }

     ///@dev Change the contract that handles bribes
    function setBribesProcessor(IBribesProcessor newBribesProcessor) external onlyGovernance {
        bribesProcessor = newBribesProcessor;
    }

It is best practice to use modifier for role based access control instead of calling a private function.

Should use solidity 0.8 instead of 0.6 with SafeMathUpgradeable

It provide more readable, more security and better gas utilization if you use solidity 0.8.

_amount.mul(9_980).div(MAX_BPS) can be replaced with _amount * 9_980 / MAX_BPS in solidity 0.8 while providing better underflow and overflow guard

checkUpkeep should check for expired lock (a way is using try catch)

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L385-L388

    function checkUpkeep(bytes calldata checkData) external view returns (bool upkeepNeeded, bytes memory performData) {
        (, uint256 unlockable, ,) = LOCKER.lockedBalances(address(this));
        upkeepNeeded = unlockable > 0;
    }

Currently, it only check for whether locker has locked fund but don't know whether it can be unlocked or not.

Should be implemented this way

    function checkUpkeep(bytes calldata checkData) external view returns (bool upkeepNeeded, bytes memory performData) {
        try LOCKER.processExpiredLocks(false); { upkeepNeeded = true; } catch (bytes memory /*lowLevelData*/) { upkeepNeeded = false; }
    }

[GalloDaSballo]

Should use modifier instead of function call for role based access control

Saves gas to use a function, it is best practice

Should use solidity 0.8 instead of 0.6 with SafeMathUpgradeable

We are happy with 0.6.12

checkUpkeep should check for expired lock (a way is using try catch)

Disagree completely it will make it more expensive and it's needlessly reverting, also not sure if you can make a non-view function callable inside a view function