QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	7
Non-Critical Risk	3

Table of Contents

• Low Risk Issues
  • 1. Add constructor initializers
  • 2. Uninitialized Upgradeable contract
  • 3. Deprecated safeApprove() function
  • 4. Lack of event emission after critical initialize() functions
  • 5. MyStrategy.sol is an upgradeable contract that is missing a __gap[50] storage variable to allow for new storage variables in later versions
  • 6. Events not indexed
  • 7. initialize() is front-runnable in MyStrategy.sol
• Non-Critical Issues
  • 1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking
  • 2. Typos
  • 3. Open TODOS

Low Risk Issues

1. Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

MyStrategy.sol:56:    function initialize(address _vault) public initializer {

2. Uninitialized Upgradeable contract

Similar issue in the past: here

Upgradeable dependencies should be initialized.
While not causing any harm at the moment, suppose OZ someday decide to upgrade this contract and the sponsor uses the new version: it is possible that the contract will not work anymore.
Consider calling the init() here:

• https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L56 :

File: MyStrategy.sol
56:     function initialize(address _vault) public initializer {
57:         assert(IVault(_vault).token() == address(AURA));
58: 
59:         __BaseStrategy_init(_vault);
+ 59:       __ReentrancyGuard_init(); //@audit ReentrancyGuardUpgradeable
60: 
61:         want = address(AURA);
62: 
63:         /// @dev do one off approvals here
64:         // Permissions for Locker
65:         AURA.safeApprove(address(LOCKER), type(uint256).max);
66: 
67:         AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
68:         WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);
69: 
70:         // Set Safe Defaults
71:         withdrawalSafetyCheck = true;
72: 
73:         // Process locks on reinvest is best left false as gov can figure out if they need to save that gas
74:     }

3. Deprecated safeApprove() function

Deprecated

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, consider replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

MyStrategy.sol:65:        AURA.safeApprove(address(LOCKER), type(uint256).max);
MyStrategy.sol:67:        AURABAL.safeApprove(address(BALANCER_VAULT), type(uint256).max);
MyStrategy.sol:68:        WETH.safeApprove(address(BALANCER_VAULT), type(uint256).max);

4. Lack of event emission after critical initialize() functions

Similar issue in the past: here

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions:

• https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L56-L74

    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);

        // Set Safe Defaults
        withdrawalSafetyCheck = true;

        // Process locks on reinvest is best left false as gov can figure out if they need to save that gas
    }

5. MyStrategy.sol is an upgradeable contract that is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

6. Events not indexed

MyStrategy.sol:51:    event RewardsCollected(address token, uint256 amount);

7. initialize() is front-runnable in MyStrategy.sol

Consider adding some access control to them or deploying atomically or using constructor initializer

Non-Critical Issues

1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

MyStrategy.sol:57:        assert(IVault(_vault).token() == address(AURA));

As the Solidity version is < 0.8.* the remaining gas would be fully used in case of failure.

2. Typos

• sweeped vs swept

MyStrategy.sol:160:    /// @notice this provides security guarantees to the depositors they can't be sweeped away

• auto-compunded vs auto-compounded

MyStrategy.sol:218:    ///      after claiming rewards or swapping are auto-compunded.

3. Open TODOS

Consider resolving the TODOs before deploying.

MyStrategy.sol:284:    // TODO: Hardcode claim.account = address(this)?
MyStrategy.sol:422:        // TODO: Too many SLOADs

[GalloDaSballo]

1. Add constructor initializers

Disagree as we initialize when we depoy the proxy

2. Uninitialized Upgradeable contract

Agree but disagree there's any impact beside gas cost on first call with the modifier

3. Deprecated safeApprove() function

Agree, but we're using it the way it was intended, once, from 0 to X

4. Lack of event emission after critical initialize() functions

Disagree

5. MyStrategy.sol is an upgradeable contract that is missing a __gap[50] storage variable to allow for new storage variables in later versions

Disagree, we got the gap in BaseStrategy

7. initialize() is front-runnable in MyStrategy.sol

Disagree per how we deploy

1. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Ack

Ack rest