QA Report

[code423n4]

• [L-01] Deprecated safeApprove() function
• [L-02] Immutable addresses should be 0-checked
• [N-01] Open TODOS
• [N-02] Code in comment
• [N-03] Related data should be grouped in a struct
• [N-04] Unused named returns
• [N-05] It's better to emit after all processing is done

[L-01] Deprecated safeApprove() function

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, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

backd/contracts/CvxCrvRewardsLocker.sol:
  53:         IERC20(CRV).safeApprove(CRV_DEPOSITOR, type(uint256).max);
  56:         IERC20(CVX_CRV).safeApprove(CVX_CRV_STAKING, type(uint256).max);
  59:         IERC20(CRV).safeApprove(CVX_CRV_CRV_CURVE_POOL, type(uint256).max);
  62:         IERC20(CVX).safeApprove(CVX_LOCKER, type(uint256).max);

backd/contracts/actions/topup/TopUpAction.sol:
   50:             IERC20(token).safeApprove(stakerVaultAddress, depositAmount);
  847:         IERC20(depositToken).safeApprove(feeHandler, feeAmount);
  908:         IERC20(token).safeApprove(spender, type(uint256).max);

backd/contracts/actions/topup/handlers/AaveHandler.sol:
  53:         IERC20(underlying).safeApprove(address(lendingPool), amount);

backd/contracts/actions/topup/handlers/CompoundHandler.sol:
   71:             IERC20(underlying).safeApprove(address(ctoken), amount);
  120:             IERC20(underlying).safeApprove(address(ctoken), debt);

backd/contracts/pool/LiquidityPool.sol:
  721:         IERC20(lpToken_).safeApprove(staker_, type(uint256).max);

backd/contracts/strategies/BkdEthCvx.sol:
  43:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);

backd/contracts/strategies/BkdTriHopCvx.sol:
   71:         IERC20(underlying_).safeApprove(curveHopPool_, type(uint256).max);
   72:         IERC20(hopLp_).safeApprove(curvePool_, type(uint256).max);
   73:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);
  129:         IERC20(hopLp).safeApprove(curvePool_, 0);
  130:         IERC20(hopLp).safeApprove(curvePool_, type(uint256).max);
  131:         IERC20(lp_).safeApprove(address(_BOOSTER), 0);
  132:         IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);

backd/contracts/strategies/ConvexStrategyBase.sol:
  107:         _CRV.safeApprove(address(_strategySwapper), type(uint256).max);
  108:         _CVX.safeApprove(address(_strategySwapper), type(uint256).max);
  109:         _WETH.safeApprove(address(_strategySwapper), type(uint256).max);
  279:         IERC20(token_).safeApprove(address(_strategySwapper), 0);
  280:         IERC20(token_).safeApprove(address(_strategySwapper), type(uint256).max);

backd/contracts/strategies/StrategySwapper.sol:
  209:         IERC20(token_).safeApprove(spender_, type(uint256).max);

backd/contracts/vault/Erc20Vault.sol:
  21:         IERC20(underlying_).safeApprove(address(reserve), type(uint256).max);
  22:         IERC20(underlying_).safeApprove(_pool, type(uint256).max);

[L-02] Immutable addresses should be 0-checked

In the constructors, the solution never checks for address(0) when setting the value of immutable variables. I suggest adding those checks.

List of immutable variables:

backd/contracts/BkdLocker.sol:
  41:     IERC20 public immutable govToken;

backd/contracts/Controller.sol:
  19:     IAddressProvider public immutable override addressProvider;

backd/contracts/GasBank.sol:
   9:     IController public immutable controller;
  10:     IAddressProvider public immutable addressProvider;

backd/contracts/StakerVault.sol:
  43:     IController public immutable controller;

backd/contracts/access/Authorization.sol:
  7:     IRoleManager internal immutable __roleManager;

backd/contracts/access/RoleManager.sol:
  23:     IAddressProvider public immutable addressProvider;

backd/contracts/actions/topup/TopUpAction.sol:
  156:     IController public immutable controller;
  157:     IAddressProvider public immutable addressProvider;

backd/contracts/actions/topup/TopUpActionFeeHandler.sol:
  31:     address public immutable actionContract;
  32:     IController public immutable controller;

backd/contracts/actions/topup/TopUpKeeperHelper.sol:
  18:     ITopUpAction private immutable _topupAction;

backd/contracts/actions/topup/handlers/AaveHandler.sol:
  21:     ILendingPool public immutable lendingPool;
  22:     IWETH public immutable weth;

backd/contracts/actions/topup/handlers/CompoundHandler.sol:
  35:     Comptroller public immutable comptroller;
  36:     ICTokenRegistry public immutable cTokenRegistry;

backd/contracts/actions/topup/handlers/CTokenRegistry.sol:
  9:     Comptroller public immutable comptroller;

backd/contracts/oracles/ChainlinkUsdWrapper.sol:
  27:     IChainlinkOracle private immutable _ethOracle =
  29:     IChainlinkOracle private immutable _oracle;
  30:     uint8 private immutable _decimals;

backd/contracts/pool/LiquidityPool.sol:
   65:     IController public immutable controller;
   66:     IAddressProvider public immutable addressProvider;

backd/contracts/pool/PoolFactory.sol:
  64:     IController public immutable controller;
  65:     IAddressProvider public immutable addressProvider;

backd/contracts/strategies/BkdTriHopCvx.sol:
  19:     ICurveSwapEth public immutable curveHopPool; // Curve Pool to use for Hops
  20:     IERC20 public immutable hopLp; // Curve Hop Pool LP Token
  21:     uint256 public immutable curveHopIndex; // Underlying index in Curve Pool

backd/contracts/strategies/ConvexStrategyBase.sol:
  43:     IStrategySwapper internal immutable _strategySwapper;
  50:     address public immutable vault; // Backd Vault

backd/contracts/strategies/StrategySwapper.sol:
  28:     IAddressProvider internal immutable _addressProvider; // Address provider used for getting oracle provider

backd/contracts/vault/Vault.sol:
  48:     IController public immutable controller;
  49:     IAddressProvider public immutable addressProvider;
  50:     IVaultReserve public immutable reserve;

[N-01] Open TODOS

Consider resolving the TODOs before deploying.

actions/topup/TopUpAction.sol:713:        // TODO: add constant gas consumed for transfer and tx prologue
strategies/ConvexStrategyBase.sol:4:// TODO Add validation of curve pools
strategies/ConvexStrategyBase.sol:5:// TODO Test validation

[N-02] Code in comment

Consider deleting the following line:

File: ILendingPool.sol
408:     // function getAddressesProvider() external view returns (ILendingPoolAddressesProvider);

[N-03] Related data should be grouped in a struct

The "LP Fee Distribution" maps should be grouped in a struct.

From:

File: BkdLocker.sol
25:     // User-specific data
26:     mapping(address => uint256) public balances;
27:     mapping(address => uint256) public boostFactors;
28:     mapping(address => uint256) public lastUpdated;
29:     mapping(address => WithdrawStash[]) public stashedGovTokens;
30:     mapping(address => uint256) public totalStashed;

To

    struct UserSpecificData {
        uint256 balances;
        uint256 boostFactors;
        uint256 lastUpdated;
        WithdrawStash[] stashedGovTokens;
        uint256 totalStashed;
    }

    mapping(address => UserSpecificData) public userSpecificData;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userSpecificData[address].

[N-04] Unused named returns

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons:

backd/contracts/actions/topup/TopUpAction.sol:
  497:         returns (address[] memory users, uint256 nextCursor) //@audit unused named returns
  761:     ) public view override returns (uint256 healthFactor) { //@audit unused named returns

backd/contracts/pool/PoolFactory.sol:
  155:     ) external onlyGovernance returns (Addresses memory addrs) { //@audit unused named returns
  216:         return addrs; //@audit unused named returns

backd/contracts/strategies/StrategySwapper.sol:
  301:         returns (uint256 wethIndex_, uint256 tokenIndex_) //@audit unused named returns
  332:         returns (uint256 minAmountOut) //@audit unused named returns

[N-05] It's better to emit after all processing is done

Instances include:

File: Vault.sol
565:     function _activateStrategy() internal returns (bool) {
566:         IStrategy strategy = getStrategy();
567:         if (address(strategy) == address(0)) return false;
568: 
569:         strategyActive = true;
570:         emit StrategyActivated(address(strategy));
571:         _deposit();
572:         return true;
573:     }