Backd contest - robee's results

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Require with not comprehensive message Severity: Low Risk

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: BkdLocker.sol, In line 136 with Require message: No entries

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    TopUpKeeperHelper.sol, getExecutableTopups
    PoolFactory.sol, deployPool
    ChainlinkUsdWrapper.sol, latestRoundData
    TopUpAction.sol, usersWithPositions
    StrategySwapper.sol, _getIndices

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    VestedEscrow.sol
    Controller.sol

Title: Open TODOs Severity: Low Risk

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Open TODO in ConvexStrategyBase.sol line 3 : // TODO Add validation of curve pools

Open TODO in ConvexStrategyBase.sol line 4 : // TODO Test validation

Open TODO in TopUpAction.sol line 712 : // TODO: add constant gas consumed for transfer and tx prologue

Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/BkdLocker.sol#L152
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/BkdTriHopCvx.sol#L172
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/swappers/Swapper3Crv.sol#L96
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/tokenomics/VestedEscrowRevocable.sol#L58
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/actions/topup/TopUpActionFeeHandler.sol#L141

Title: Hardcoded WETH Severity: Low Risk

WETH address is hardcoded but it may differ on other chains, e.g. Polygon, so make sure to check this before deploying and update if necessary You should consider injecting WETH address via the constructor. (previous issue: https://github.com/code-423n4/2021-10-ambire-findings/issues/54) Hardcoded weth address in ConvexStrategyBase.sol Hardcoded weth address in Swapper3Crv.sol Hardcoded weth address in StrategySwapper.sol

Title: Missing fee parameter validation Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    Vault.preparePerformanceFee (newPerformanceFee)
    Vault.prepareReserveFee (newReserveFee)
    TopUpAction.prepareActionFee (newActionFee)
    TopUpActionFeeHandler.prepareTreasuryFee (newTreasuryFee)
    Vault.prepareStrategistFee (newStrategistFee)

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:
        
        Instead a < b / c use a * c < b. 
    
In all of the big and trusted contracts this rule is maintained.

    LiquidityPool.sol, 208, require(newRatio <= (ScaledMath.DECIMAL_SCALE * 50) / 100, Error.INVALID_AMOUNT);

Title: Add a timelock Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/StrategySwapper.sol#L122
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/oracles/ChainlinkOracleProvider.sol#L29
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/StrategySwapper.sol#L109
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/tokenomics/VestedEscrow.sol#L69
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/CvxCrvRewardsLocker.sol#L119

Title: Treasury may be address(0) Severity: Low Risk

Make sure the treasury is not address(0).
    
    
    CvxCrvRewardsLocker.sol.setTreasury _treasury

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in BkdTriHopCvx.sol line 72: IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);

Deprecated safeApprove in BkdTriHopCvx.sol line 128: IERC20(hopLp).safeApprove(curvePool_, 0);

Deprecated safeApprove in ConvexStrategyBase.sol line 278: IERC20(token_).safeApprove(address(_strategySwapper), 0);

Deprecated safeApprove in TopUpAction.sol line 846: IERC20(depositToken).safeApprove(feeHandler, feeAmount);

Deprecated safeApprove in CompoundHandler.sol line 119: IERC20(underlying).safeApprove(address(ctoken), debt);

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    ExponentialNoError.sol: function sub_ parameter b isn't used. (sub_ is internal)
    EthVault.sol: function _payStrategist parameter amount isn't used. (_payStrategist is internal)
    EthVault.sol: function _depositToReserve parameter amount isn't used. (_depositToReserve is internal)
    ExponentialNoError.sol: function add_ parameter a isn't used. (add_ is internal)
    ExponentialNoError.sol: function fraction parameter a isn't used. (fraction is internal)

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    LpToken.sol.burn owner

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    LpToken.sol, initialize, 28
    AddressProvider.sol, initialize, 53
    EthVault.sol, initialize, 15
    Erc20Vault.sol, initialize, 12
    TopUpAction.sol, initialize, 180

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    AddressProvider.removeFeeHandler (feeHandler)
    TopUpActionFeeHandler.constructor (treasuryFee)
    ChainlinkOracleProvider.constructor (ethFeed)
    Vault._shareFees (totalFeeAmount)
    Vault._checkFeesInvariant (strategistFee)

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    AddressProvider.sol, updateVault

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    InflationManager.sol.removeKeeperGauge pool
    VestedEscrowRevocable.sol.claim _recipient
    TopUpAction.sol.addUsableToken token
    StakerVault.sol.stakeFor account
    InflationManager.sol.advanceKeeperGaugeEpoch pool

Hope this report will help you to optimize your codebase, I think there are here some cool stuff :)

Title: Unnecessary default assignment Severity: GAS

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

    LiquidityPool.sol (L#52) : uint256 internal constant _INITIAL_MAX_WITHDRAWAL_FEE = 0.03e18; 
    Vault.sol (L#45) : uint256 public constant MAX_DEVIATION_BOUND = 0.5e18;
    LiquidityPool.sol (L#50) : uint256 internal constant _INITIAL_RESERVE_DEVIATION = 0.005e18;
    Vault.sol (L#44) : uint256 public constant MAX_PERFORMANCE_FEE = 0.5e18;
    CvxCrvRewardsLocker.sol (L#43) : int128 private constant _CRV_INDEX = 0;

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In VestedEscrow.sol,rearranging the storage fields can optimize to: 8 slots from: 9 slots. The new order of types (you choose the actual variables): 1. IERC20 2. uint256 3. uint256 4. uint256 5. uint256 6. uint256 7. address 8. bool 9. address

In KeeperGauge.sol,rearranging the storage fields can optimize to: 3 slots from: 4 slots. The new order of types (you choose the actual variables): 1. IController 2. uint256 3. address 4. uint48 5. bool

Title: More efficient Struct packing of Market in the contract ComptrollerStorage.sol Severity: GAS

    The following structs could change the order of their stored elements to decrease memory uses.
    and number of occupied slots. Therefore will save gas at every store and load from memory.
    

In ComptrollerStorage.sol, Market is optimized to: 3 slots from: 4 slots. The new order of types (you choose the actual variables): 1. uint256 2. mapping 3. bool 4. bool

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: InflationManager.sol, i, 91
    change to prefix increment and unchecked: EnumerableExtensions.sol, i, 118
    change to prefix increment and unchecked: InflationManager.sol, i, 166
    change to prefix increment and unchecked: CompoundHandler.sol, i, 135
    change to prefix increment and unchecked: InflationManager.sol, i, 191

Title: Unused declared local variables Severity: GAS

Unused local variables are gas consuming, since the initial value assignment costs gas. And are a bad code practice. Removing those variables will decrease the gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    CompoundHandler.sol, _repayAnyDebt, cether

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: Minter.sol, In line 143, Require message length to shorten: 38, The message: Maximum non-inflation amount exceeded.

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    Erc20Vault.sol.constructor
    SwapperRegistry.sol.constructor
    Erc20Pool.sol.constructor
    MockStrategySwapper.sol.constructor
    EthVault.sol.constructor

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    Errors.sol, ADMIN_ALREADY_SET
    Errors.sol, CANNOT_REVOKE_ROLE
    Errors.sol, UNAUTHORIZED_PAUSE
    Errors.sol, INSUFFICIENT_BALANCE
    Errors.sol, EXCEEDS_DEPOSIT_CAP

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    InflationManager.sol, stakerVaults, 109
    RoleManager.sol, roles, 80
    VestedEscrow.sol, amounts, 93
    CTokenRegistry.sol, ctokens, 61
    CompoundHandler.sol, assets, 135

Title: Unnecessary payable Severity: GAS

The following functions are payable but msg.value isn't used - therefore the function payable state modifier isn't necessary. Payable functions are more gas expensive than others, and it's danger the users if they send ETH by mistake.

    ConvexStrategyBase.sol, deposit is payable but doesn't use msg.value
    LiquidityPool.sol, deposit is payable but doesn't use msg.value
    LiquidityPool.sol, depositFor is payable but doesn't use msg.value
    Vault.sol, deposit is payable but doesn't use msg.value
    LiquidityPool.sol, depositAndStake is payable but doesn't use msg.value

Title: Unnecessary cast Severity: Gas

    IController PoolFactory.sol.constructor - unnecessary casting IController(_controller)
    IController LpGauge.sol.constructor - unnecessary casting IController(_controller)
    IController LiquidityPool.sol.constructor - unnecessary casting IController(_controller)

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    minWithdrawalDelay in VaultReserve.sol
    token in StakerVault.sol
    minter in LpToken.sol
    name in LiquidityPool.sol
    _decimals in LpToken.sol

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    StakerVault.sol, _isAuthorizedToPause
    Erc20Vault.sol, _payStrategist
    AddressProviderHelpers.sol, getRoleManager
    ExponentialNoError.sol, lessThanOrEqualExp
    Erc20Vault.sol, _availableUnderlying

Title: Unnecessary equals boolean Severity: GAS

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    CvxCrvRewardsLocker.sol, 276: if (IERC20(CVX_CRV).balanceOf(address(this)) == 0) return false;
    CompoundHandler.sol, 64: if (amount == 0) return true;
    AaveHandler.sol, 61: if (amount == 0) return true;
    Vault.sol, 567: if (address(strategy) == address(0)) return false;
    RoleManager.sol, 135: account == addressProvider.getAddress(AddressProviderKeys._POOL_FACTORY_KEY, false);

Title: Change if -> revert pattern to require Severity: GAS

Change if -> revert pattern to 'require' to save gas and improve code quality, if (some_condition) { revert(revert_message) }

to: require(!some_condition, revert_message)

In the following locations:

    CTokenRegistry.sol, 51

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    StakerVault.sol, line 16, import "../interfaces/tokenomics/IRewardsGauge.sol";
    IAddressProvider.sol, line 5, import "./IGasBank.sol";
    StakerVault.sol, line 9, import "../libraries/Errors.sol";
    AddressProvider.sol, line 8, import "../interfaces/oracles/IOracleProvider.sol";
    PoolFactory.sol, line 3, import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    KeeperGauge.sol, _calcTotalClaimable
    Erc20Vault.sol, _payStrategist
    Swapper3Crv.sol, _getBestTokenToWithdraw
    Vault.sol, _handleExcessDebt
    StrategySwapper.sol, _approve

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    BkdLocker.sol (L#275) - newBoost += (block.timestamp - lastUpdated[user]) .scaledDiv(currentUInts256[_INCREASE_PERIOD]) .scaledMul(maxBoost - startBoost);
    LpGauge.sol (L#70) - poolStakedIntegral_ += (inflationManager.getLpRateForStakerVault(address(stakerVault)) * (block.timestamp - poolLastUpdate)).scaledDiv(poolTotalStaked);
    VaultReserve.sol (L#103) - return block.timestamp >= _lastWithdrawal[vault] + minWithdrawalDelay;
    Minter.sol (L#181) - totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
    AmmGauge.sol (L#89) - ammStakedIntegral_ += (controller.inflationManager().getAmmRateForToken(ammToken) * (block.timestamp - uint256(ammLastUpdated))).scaledDiv(totalStaked);

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    Errors.sol (L37), string internal constant ROLE_EXISTS = "role already exists";
    Errors.sol (L78), string internal constant GAS_BANK_BALANCE_TOO_LOW = "not enough ETH in gas bank to cover gas";
    Errors.sol (L44), string internal constant THRESHOLD_TOO_HIGH = "threshold is too high, must be under 10";
    Errors.sol (L85), string internal constant STRATEGY_DOES_NOT_EXIST = "Strategy does not exist";
    Errors.sol (L31), string internal constant INSUFFICIENT_BALANCE = "insufficient balance";

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    LiquidityPool.sol, 401: change 'depositCap > 0' to 'depositCap != 0'
    LiquidityPool.sol, 401: change '_depositCap > 0' to '_depositCap != 0'
    AmmGauge.sol, 125: change 'amount > 0' to 'amount != 0'
    BkdLocker.sol, 91: change 'totalLockedBoosted > 0' to 'totalLockedBoosted != 0'
    AmmConvexGauge.sol, 173: change 'balance > 0' to 'balance != 0'

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    TopUpAction.sol, 506
    TopUpKeeperHelper.sol, 72
    EnumerableExtensions.sol, 21
    Controller.sol, 117
    VestedEscrow.sol, 93

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    Swapper3Crv.sol, _getBestTokenToWithdraw
    BkdTriHopCvx.sol, _maxLpBurned
    Vault.sol, _depositToReserve
    LiquidityPool.sol, _doTransferIn
    CompoundHandler.sol, _repayAnyDebt

Title: Do not cache msg.sender Severity: Gas

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/tokenomics/VestedEscrow.sol#L64
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/StakerVault.sol#L187
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/StakerVault.sol#L140
    https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/actions/topup/TopUpAction.sol#L281

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    ExponentialNoError.sub_ (a)
    LiquidityPool._initialize (name_)
    ExponentialNoError.mul_ScalarTruncate (a)
    AddressProvider._initializeAddress (meta)
    ExponentialNoError.sub_ (b)

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    TopUpAction.sol; the inherited contracts Authorization not used
    Minter.sol; the inherited contracts Authorization not used
    PoolFactory.sol; the inherited contracts Authorization not used
    StrategySwapper.sol; the inherited contracts Authorization not used
    BkdLocker.sol; the inherited contracts Authorization not used

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    ScaledMath.sol, scaledMul, { return (a * b) / DECIMAL_SCALE; }
    ConvexStrategyBase.sol, _lpBalance, { return lp.balanceOf(address(this)); }
    StrategySwapper.sol, _getIndices, { return curvePool_.coins(1) == token_ ? (0, 1) : (1, 0); }
    TopUpAction.sol, _getSwapper, { return TopUpActionLibrary.getSwapper(addressProvider, underlying, actionToken); }
    TopUpKeeperHelper.sol, _canExecute, { return canExecute(ITopUpAction.RecordKey(user, position.account, position.protocol)); }

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    TopUpAction.sol, prepareTopUpHandler
    EthPool.sol, getUnderlying
    Erc20Pool.sol, initialize
    Erc20Vault.sol, getUnderlying
    StakerVault.sol, unstake

Title: Cache powers of 10 used several times Severity: GAS

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

DecimalScale.sol, 12 : You should cache the used power of 10 as constant state variable since it's used several times (2): return value * 10**(DECIMALS - decimals);

DecimalScale.sol, 10 : You should cache the used power of 10 as constant state variable since it's used several times (2): return value / 10**(decimals - DECIMALS);