Badger-Vested-Aura contest - sorrynotsorry's results

QA (LOW & NON-CRITICAL)

[L-01] Outdated compiler version

Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs. The scoped contract's compiler version is 0.6.12 Current Solidity version available is 0.8.15

[L-02] Deprecated safeApprove() function

The OpenZeppelin ERC20 safeApprove() function has been deprecated.A deeper discussion on the deprecation of this function is in OZ issue #2219. The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code. The deprecated function is found in:

function initialize(address _vault) public initializer {
        assert(IVault(_vault).token() == address(AURA));

        __BaseStrategy_init(_vault);

        want = address(AURA);

        /// @dev do one off approvals here
        // Permissions for Locker
        AURA.safeApprove(address(LOCKER), type(uint256).max);

        AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
        WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

    }

[L-03] Critical address / parameter changes

Critical address changes are not done in two steps for the following methods: manualSetDelegate(), setBribesProcessor()

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference

[L-04] Missing of zero address checks

No address(0) or Zero value check for the followings:

function manualSetDelegate(address delegate) external {

function setBribesProcessor(IBribesProcessor newBribesProcessor) external {

[L-05] Questionable visibility

The _withdrawSome, _harvest functions have the visibility of internal However, there are no functions calling it on the same contract unless any contract will inherit MyStrategy.sol

[L-06] Variable shadowing in _withdrawSome()

The _withdrawSome function is reverting with the max withdrawable amount. However, the local variables are being shadowed creating redundant statemens. Please check @audit-info comments below;

function _withdrawSome(uint256 _amount) internal override returns (uint256) {
        uint256 max = balanceOfWant(); // @audit-info 1. max variable is initialized and cached to Aura Balance

        if (_amount > max) {
            // Try to unlock, as much as possible
            // @notice Reverts if no locks expired
            LOCKER.processExpiredLocks(false);
            max = balanceOfWant(); //_amount = balanceofWant() // @audit-info 2. max variable is re-cached to Aura Balance. Should it be _amount = balanceofWant() ?
        }

        if (withdrawalSafetyCheck) { 
            require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage
        }

        if (_amount > max) { // @audit-info This comparison is already done at the first if block. It can be inlined in the first block.
            return max;
        }

        return _amount;
    }

[L-07] Boundries for _harvest()

_harvest function executes the swaps according to auraBalEarned > 0. But for the sake of clearity and being user friendly, there should be bounds for this comparison. Else, the method can even burn more gas than the value to be harvested. In theory; auraBalEarned should be proportionally greater than gasleft().

[L-08] Unbounded loop on array can lead to DoS

Programming patterns such as looping over arrays of unknown size may lead to DoS when the gas cost of execution exceeds the block gas limit. Reference

Instances;

1. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300-L307
2. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317-L327

[L-09] External Calls inside a loop

Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded.Reference

Instances;

1. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300-L306
2. https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317-L334

[N-01] Scoped contracts are missing proper NatSpec comments

The scoped contracts are missing proper NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference

[N-02] TODO and TODO like comments

The team might consider to remove TODO and TODO like comments as below;

1. ///@dev Should we check if the amount requested is more than what we can return on withdrawal?
2. ///@dev Should we processExpiredLocks during reinvest?
3. // TODO: Too many SLOADs
4. /// @dev Does this function require tend to be called?
5. // Change to true if the strategy should be tended
6. // Make sure to call prepareWithdrawAll before _withdrawAll
7. // TODO: Hardcode claim.account = address(this)?