Badger-Vested-Aura contest - codexploder's results

checkUpkeep should be called within performUpkeep

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L391

Issue: The performUpkeep can use checkUpkeep to understand if upkeepNeeded is needed or not. Otherwise performUpkeep will simply fail

Recommendation: Add below check:

function performUpkeep(bytes calldata performData) external {
       (bool check,) =checkUpkeep(performData);
if(check)
        LOCKER.processExpiredLocks(false);
    }

Use variables instead of hardcoding

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L136

Issue: In function _isTendable, Instead of hardcoding false/true, use a variable which can allow/disallow tending on calling this function

Recommendation: Add a new function like below:

bool tendable;

function setTendable(bool value) internal pure override returns (bool) {
_onlyGovernanceOrStrategist();
        tendable=value;
    }

function _isTendable() internal pure override returns (bool) {
        return tendable; // Change to true if the strategy should be tended
    }

Incorrect Function implementation

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L107

Issue: In function sweepRewardToken, Comment mentions "because token paths are hardcoded, this function is safe to be called by anyone" but still restriction is placed and this function is only callable by _onlyGovernanceOrStrategist

Recommendation: If this is meant for external use remove the restriction for _onlyGovernanceOrStrategist

Zero address checks missing

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L79

Issue: In function manualSetDelegate, Zero address check is missing while setting delegate. Same fix required for setBribesProcessor

Recommendation: Check if provided delegate is not address(0)

require(delegate!=address(0);,"Incorrect address");

WithdrawAll does not withdraw anything

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L182

Issue: The name of function is giving wrong impression. This function is not withdrawing anything and is just checking whether contract holding is empty now.

Recommendation: Change the function name to isHoldingEmpty or something which clearly states functionality

Multiple function performing same operation

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L1

Issue: Too many functions are present which are processing locked balances like manualProcessExpiredLocks, performUpkeep.

Recommendation: Remove extra function like manualProcessExpiredLocks which can be replaced with performUpkeep

Function can be marked view to save gas

Contract: https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L396

Issue: The _getBalance and _getPricePerFullShare can be marked as view

Recommendation: Change the function as below:

function _getBalance() internal view returns (uint256) {
        return IVault(vault).balance();
    }

    function _getPricePerFullShare() internal view returns (uint256) {
        return IVault(vault).getPricePerFullShare();
    }

Use ++i instead of i++

Make use of ++i which is unchecked instead of i++ in loops. This will prevent gas on each loop run. One such example is at MyStrategy.sol#L116