QA Report #13

code423n4 · 2022-05-29T18:20:15Z

Mult instead div in compares

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.

Code instance:

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

Missing fee parameter validation

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

Code instances:

    Vault.prepareReserveFee (newReserveFee)
    Vault.prepareStrategistFee (newStrategistFee)
    TopUpActionFeeHandler.prepareKeeperFee (newKeeperFee)
    TopUpActionFeeHandler.prepareTreasuryFee (newTreasuryFee)
    LiquidityPool.prepareNewMaxWithdrawalFee (newFee)
    LiquidityPool.prepareNewMinWithdrawalFee (newFee)
    Vault.preparePerformanceFee (newPerformanceFee)

Does not validate the input fee parameter

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

Code instances:

    AddressProvider.removeFeeHandler (feeHandler)
    AddressProvider.addFeeHandler (feeHandler)
    Vault._checkFeesInvariant (reserveFee)
    ChainlinkOracleProvider.constructor (feedRegistry_)
    Vault._checkFeesInvariant (strategistFee)
    LiquidityPool._checkFeeInvariants (minFee)
    LiquidityPool.getNewCurrentFees (feeRatio)
    TopUpActionFeeHandler.constructor (keeperFee)
    Vault._shareFees (totalFeeAmount)
    TopUpActionFeeHandler.constructor (treasuryFee)
    AddressProvider.isWhiteListedFeeHandler (feeHandler)

safeApprove of openZeppelin is deprecated

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.

Code instances:

    Deprecated safeApprove in LiquidityPool.sol line 699: IERC20(lpToken_).safeApprove(staker_, type(uint256).max);
    Deprecated safeApprove in ConvexStrategyBase.sol line 288: IERC20(token_).safeApprove(_swapperRouter, type(uint256).max);
    Deprecated safeApprove in RewardHandler.sol line 63: IERC20(token).safeApprove(spender, type(uint256).max);
    Deprecated safeApprove in AmmConvexGauge.sol line 60: IERC20(ammToken).safeApprove(booster, type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 131: IERC20(hopLp).safeApprove(curvePool_, 0);
    Deprecated safeApprove in Erc20Vault.sol line 21: IERC20(underlying_).safeApprove(_pool, type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 72: IERC20(hopLp_).safeApprove(curvePool_, type(uint256).max);
    Deprecated safeApprove in ConvexStrategyBase.sol line 104: _CRV.safeApprove(address(swapperRouter_), type(uint256).max);
    Deprecated safeApprove in AaveHandler.sol line 84: IERC20(token).safeApprove(spender, type(uint256).max);
    Deprecated safeApprove in ConvexStrategyBase.sol line 105: _CVX.safeApprove(address(swapperRouter_), type(uint256).max);
    Deprecated safeApprove in CvxCrvRewardsLocker.sol line 65: IERC20(CVX).safeApprove(CVX_LOCKER, type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 132: IERC20(hopLp).safeApprove(curvePool_, type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 134: IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);
    Deprecated safeApprove in CompoundHandler.sol line 137: IERC20(token).safeApprove(spender, type(uint256).max);
    Deprecated safeApprove in PoolMigrationZap.sol line 26: IERC20(underlying_).safeApprove(address(newPool_), type(uint256).max);
    Deprecated safeApprove in RewardHandler.sol line 51: IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount);
    Deprecated safeApprove in FeeBurner.sol line 117: IERC20(token_).safeApprove(spender_, type(uint256).max);
    Deprecated safeApprove in ConvexStrategyBase.sol line 106: _WETH.safeApprove(address(swapperRouter_), type(uint256).max);
    Deprecated safeApprove in Erc20Vault.sol line 20: IERC20(underlying_).safeApprove(address(reserve), type(uint256).max);
    Deprecated safeApprove in ConvexStrategyBase.sol line 287: IERC20(token_).safeApprove(_swapperRouter, 0);
    Deprecated safeApprove in CvxCrvRewardsLocker.sol line 59: IERC20(CVX_CRV).safeApprove(CVX_CRV_STAKING, type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 133: IERC20(lp_).safeApprove(address(_BOOSTER), 0);
    Deprecated safeApprove in BkdTriHopCvx.sol line 73: IERC20(lp_).safeApprove(address(_BOOSTER), type(uint256).max);
    Deprecated safeApprove in BkdTriHopCvx.sol line 71: IERC20(underlying_).safeApprove(curveHopPool_, type(uint256).max);
    Deprecated safeApprove in CvxCrvRewardsLocker.sol line 56: IERC20(CRV).safeApprove(CRV_DEPOSITOR, type(uint256).max);
    Deprecated safeApprove in VestedEscrow.sol line 24: IERC20(rewardToken_).approve(msg.sender, type(uint256).max);
    Deprecated safeApprove in CvxCrvRewardsLocker.sol line 62: IERC20(CRV).safeApprove(CVX_CRV_CRV_CURVE_POOL, type(uint256).max);
    Deprecated safeApprove in SwapperRouter.sol line 264: IERC20(token_).safeApprove(spender_, type(uint256).max);

Require with not comprehensive message

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:

Code instance:

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

Not verified input

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.

Code instances:

    TopUpActionFeeHandler.sol.claimKeeperFeesForPool beneficiary
    Minter.sol.setToken _token
    InflationManager.sol.executeKeeperPoolWeight pool
    StakerVault.sol.stakeFor account
    AddressProvider.sol.addPool pool
    TopUpActionFeeHandler.sol.prepareKeeperGauge newKeeperGauge
    VestedEscrowRevocable.sol.claim _recipient
    InflationManager.sol.addGaugeForVault lpToken
    InflationManager.sol.executeLpPoolWeight lpToken
    LpGauge.sol.userCheckpoint user
    StakerVault.sol.decreaseActionLockedBalance account
    StakerVault.sol.unstakeFor dst
    GasBank.sol.withdrawFrom account
    ConvexStrategyBase.sol.removeRewardToken token_
    TopUpActionFeeHandler.sol.payFees beneficiary
    AmmGauge.sol.stakeFor account
    PoolFactory.sol.addLpTokenImplementation implementation
    RoleManager.sol.removeGaugeZap zap
    BkdTriHopCvx.sol.changeConvexPool curvePool_
    AddressProvider.sol.prepareAddress newAddress
    Erc20Vault.sol.initialize _pool
    EthVault.sol.initialize _pool
    CvxCrvRewardsLocker.sol.clearDelegate delegateContract
    StakerVault.sol.prepareLpGauge _lpGauge
    InflationManager.sol.setAmmGauge _ammGauge
    KeeperGauge.sol.reportFees beneficiary
    VestedEscrowRevocable.sol.revoke _recipient
    StakerVault.sol.initialize _token
    TopUpActionFeeHandler.sol.prepareKeeperGauge lpToken
    AmmGauge.sol.claimRewards beneficiary
    SwapperRouter.sol.setCurvePool curvePool_
    AddressProvider.sol.initializeAddress initialAddress
    ConvexStrategyBase.sol.addRewardToken token_
    LiquidityPool.sol.handleLpTokenTransfer from
    Vault.sol.initializeStrategy strategy_
    LiquidityPool.sol.prepareNewVault _vault
    PoolFactory.sol.addStakerVaultImplementation implementation
    InflationManager.sol.setAmmGauge token
    CvxCrvRewardsLocker.sol.setDelegate delegateContract
    Controller.sol.setInflationManager _inflationManager
    LiquidityPool.sol.handleLpTokenTransfer to
    RoleManager.sol.grantRole account
    BkdLocker.sol.lockFor user
    StakerVault.sol.transferFrom dst
    InflationManager.sol.prepareKeeperPoolWeight pool
    Minter.sol.mintNonInflationTokens beneficiary
    InflationManager.sol.addStrategyToDepositStakerVault depositStakerVault
    AddressProvider.sol.removeFeeHandler feeHandler
    LpToken.sol.initialize _minter
    Controller.sol.removePool pool
    PoolFactory.sol.addVaultImplementation implementation
    AmmConvexGauge.sol.claimRewards beneficiary
    BkdLocker.sol.userCheckpoint user
    AddressProvider.sol.addAction action
    InflationManager.sol.removeStakerVaultFromInflation lpToken
    InflationManager.sol.setKeeperGauge _keeperGauge
    LiquidityPool.sol.setLpToken _lpToken
    GasBank.sol.depositFor account
    AmmGauge.sol.unstakeFor dst
    GasBank.sol.withdrawUnused account
    StakerVault.sol.transfer account
    InflationManager.sol.setMinter _minter
    SwapperRouter.sol.setCurvePool token_
    StakerVault.sol.transferFrom src
    AddressProvider.sol.initializeAndFreezeAddress initialAddress
    KeeperGauge.sol.claimRewards beneficiary
    InflationManager.sol.advanceKeeperGaugeEpoch pool
    TopUpActionFeeHandler.sol.setInitialKeeperGaugeForToken lpToken
    AddressProvider.sol.updateVault newVault
    CvxCrvRewardsLocker.sol.withdraw token
    VestedEscrow.sol.setAdmin _admin
    CvxCrvRewardsLocker.sol.setDelegate delegate
    LpToken.sol.mint account
    EthPool.sol.initialize vault_
    CvxCrvRewardsLocker.sol.forfeitRewards token
    VestedEscrow.sol.setFundAdmin _fundadmin
    PoolFactory.sol.addPoolImplementation implementation
    InflationManager.sol.executeAmmTokenWeight token
    Minter.sol.mint beneficiary
    Controller.sol.addStakerVault stakerVault
    SwapperRouter.sol.swapAll toToken_
    InflationManager.sol.removeKeeperGauge pool
    InflationManager.sol.removeStakerVaultFromInflation stakerVault
    PoolMigrationZap.sol.migrate oldPoolAddress_
    RoleManager.sol.addGaugeZap zap
    Vault.sol.withdrawFromStrategyWaitingForRemoval strategy
    KeeperGauge.sol.reportFees lpTokenAddress
    VestedEscrow.sol.claim _recipient
    TopUpActionFeeHandler.sol.executeKeeperGauge lpToken
    InflationManager.sol.addStrategyToDepositStakerVault strategyPool
    AddressProvider.sol.removePool pool
    GasBank.sol.withdrawFrom to
    TopUpActionFeeHandler.sol.payFees payer
    RoleManager.sol.addGovernor newGovernor
    LiquidityPool.sol.depositFor account
    InflationManager.sol.setKeeperGauge pool
    Erc20Pool.sol.initialize underlying_
    ConvexStrategyBase.sol.setStrategist strategist_
    BkdToken.sol.mint account
    FeeBurner.sol.burnToTarget targetLpToken_
    ConvexStrategyBase.sol.setCommunityReserve _communityReserve
    Erc20Pool.sol.initialize vault_
    BkdLocker.sol.migrate newRewardToken
    StakerVault.sol.addStrategy strategy
    InflationManager.sol.whitelistGauge gauge
    AddressProvider.sol.updateVault previousVault
    TopUpActionFeeHandler.sol.setInitialKeeperGaugeForToken _keeperGauge
    TopUpActionFeeHandler.sol.payFees lpTokenAddress
    InflationManager.sol.removeAmmGauge token
    AmmConvexGauge.sol.unstakeFor dst
    InflationManager.sol.prepareLpPoolWeight lpToken
    StakerVault.sol.approve spender
    AddressProvider.sol.addStakerVault stakerVault
    AmmConvexGauge.sol.stakeFor account
    InflationManager.sol.mintRewards beneficiary
    AddressProvider.sol.initialize roleManager
    TopUpActionFeeHandler.sol.claimKeeperFeesForPool token
    Vault.sol.prepareNewStrategy newStrategy
    StakerVault.sol.initializeLpGauge _lpGauge
    TopUpActionFeeHandler.sol.claimTreasuryFees token
    AmmConvexGauge.sol.setInflationRecipient recipient
    InflationManager.sol.prepareAmmTokenWeight token
    TopUpActionFeeHandler.sol.resetKeeperGauge lpToken
    CTokenRegistry.sol.fetchCToken underlying
    StakerVault.sol.increaseActionLockedBalance account
    AddressProvider.sol.addFeeHandler feeHandler
    LpGauge.sol.claimRewards beneficiary

Treasury may be address(0)

Make sure the treasury is not address(0).

Code instance:

    CvxCrvRewardsLocker.sol.setTreasury _treasury

Solidity compiler versions mismatch

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

Code instance:

Hardcoded WETH

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: code-423n4/2021-10-ambire-findings#54)

Code instances:

    Hardcoded weth address in SwapperRouter.sol
    Hardcoded weth address in FeeBurner.sol
    Hardcoded weth address in ConvexStrategyBase.sol

Not verified owner

    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.

Code instance:

    LpToken.sol.burn owner

Init frontrun

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: code-423n4/2021-09-sushimiso-findings#64)
The vulnerable initialization functions in the codebase are:

Code instances:

    EthVault.sol, initialize, 15
    StakerVault.sol, initialize, 71
    LpToken.sol, initialize, 28
    AddressProvider.sol, initialize, 53
    LiquidityPool.sol, _initialize, 681
    Erc20Vault.sol, initialize, 12

Named return issue

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.

Code instances:

    TopUpKeeperHelper.sol, getExecutableTopups
    SwapperRouter.sol, swapAll
    SwapperRouter.sol, _minTokenAmountOut
    SwapperRouter.sol, _swapForWeth
    SwapperRouter.sol, _returnTokens
    SwapperRouter.sol, _swapWethForToken
    SwapperRouter.sol, _getPriceInEth
    SwapperRouter.sol, swap
    SwapperRouter.sol, _minWethAmountOut
    PoolFactory.sol, deployPool
    SwapperRouter.sol, _getWethOut
    SwapperRouter.sol, _tokenAmountOut
    SwapperRouter.sol, _getIndices
    SwapperRouter.sol, _getTokenOut
    SwapperRouter.sol, getAmountOut
    FeeBurner.sol, burnToTarget
    SwapperRouter.sol, _swap
    FeeBurner.sol, _depositInPool

Two Steps Verification before Transferring Ownership

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

Code instances:

    VestedEscrow.sol
    Controller.sol
    IVestedEscrow.sol

In the following public update functions no value is returned

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: code-423n4/2021-10-badgerdao-findings#85).

Code instance:

    AddressProvider.sol, updateVault

Never used parameters

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.

Code instances:

    EthPool.sol: function _doTransferOut parameter to isn't used. (_doTransferOut is internal)
    ExponentialNoError.sol: function sub_ parameter a isn't used. (sub_ is internal)
    EthPool.sol: function _doTransferOut parameter amount isn't used. (_doTransferOut is internal)
    EthVault.sol: function _depositToReserve parameter amount isn't used. (_depositToReserve is internal)
    EthVault.sol: function _depositToRewardHandler parameter amount isn't used. (_depositToRewardHandler is internal)
    EthVault.sol: function _transfer parameter amount isn't used. (_transfer is internal)
    ExponentialNoError.sol: function fraction parameter b isn't used. (fraction is internal)
    ExponentialNoError.sol: function sub_ parameter b isn't used. (sub_ is internal)
    ExponentialNoError.sol: function add_ parameter b isn't used. (add_ is internal)
    EthVault.sol: function _payStrategist parameter amount isn't used. (_payStrategist is internal)
    EthVault.sol: function _transfer parameter to isn't used. (_transfer 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)

Missing commenting

    The following functions are missing commenting as describe below:

Code instance:

    RewardHandler.sol, _approve (internal), parameters token, spender not commented

Check transfer receiver is not 0 to avoid burned money

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

Code instances:

    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/CvxCrvRewardsLocker.sol#L175
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L112
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/swappers/SwapperRouter.sol#L280
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/handlers/CompoundHandler.sol#L122
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/TopUpActionFeeHandler.sol#L127
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/swappers/SwapperRouter.sol#L140
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L80
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/pool/Erc20Pool.sol#L34
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/Vault.sol#L144
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L228
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L93
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L153
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L70
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L144
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/TopUpActionFeeHandler.sol#L141
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L162
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/CvxCrvRewardsLocker.sol#L231
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/handlers/CompoundHandler.sol#L73
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/strategies/ConvexStrategyBase.sol#L414
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/VaultReserve.sol#L56
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L131
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/VaultReserve.sol#L85
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/TopUpActionFeeHandler.sol#L93
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L146
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L378
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/CvxCrvRewardsLocker.sol#L291
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/pool/Erc20Pool.sol#L30
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/strategies/BkdTriHopCvx.sol#L175
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L102
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/LpToken.sol#L87
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/Vault.sol#L153
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L109
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/Vault.sol#L804
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/strategies/BkdTriHopCvx.sol#L222
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/actions/topup/handlers/AaveHandler.sol#L48
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/Erc20Vault.sol#L42
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrowRevocable.sol#L60
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/zaps/PoolMigrationZap.sol#L58
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L334
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L177
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/vault/Erc20Vault.sol#L30
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L217

In the following public update functions no value is returned

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: code-423n4/2021-10-badgerdao-findings#85).

Code instance:

    AddressProvider.sol, updateVault

Never used parameters

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.

Code instances:

    EthPool.sol: function _doTransferOut parameter to isn't used. (_doTransferOut is internal)
    ExponentialNoError.sol: function sub_ parameter a isn't used. (sub_ is internal)
    EthPool.sol: function _doTransferOut parameter amount isn't used. (_doTransferOut is internal)
    EthVault.sol: function _depositToReserve parameter amount isn't used. (_depositToReserve is internal)
    EthVault.sol: function _depositToRewardHandler parameter amount isn't used. (_depositToRewardHandler is internal)
    EthVault.sol: function _transfer parameter amount isn't used. (_transfer is internal)
    ExponentialNoError.sol: function fraction parameter b isn't used. (fraction is internal)
    ExponentialNoError.sol: function sub_ parameter b isn't used. (sub_ is internal)
    ExponentialNoError.sol: function add_ parameter b isn't used. (add_ is internal)
    EthVault.sol: function _payStrategist parameter amount isn't used. (_payStrategist is internal)
    EthVault.sol: function _transfer parameter to isn't used. (_transfer 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)

Missing commenting

    The following functions are missing commenting as describe below:

Code instance:

    RewardHandler.sol, _approve (internal), parameters token, spender not commented

Add a timelock

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

Code instances:

    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L74
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L99
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/oracles/ChainlinkOracleProvider.sol#L34
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/swappers/SwapperRouter.sol#L81
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/CvxCrvRewardsLocker.sol#L123
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L33
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/swappers/SwapperRouter.sol#L93
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/CvxCrvRewardsLocker.sol#L190
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L68
    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L86

The text was updated successfully, but these errors were encountered:

GalloDaSballo · 2022-06-21T17:16:23Z

Context: at this point Robee is the turning test of QA reports as they definitely rely on regex or tools to scrape the code

Mult instead div in compares

I think the finding has some validity but in this context it won't make a huge difference

Missing fee parameter validation

Disagree per _checkFeesInvariant

safeApprove of openZeppelin is deprecated

True but the sponsor has used safeApprove in the intended way

Require with not comprehensive message

Would have liked a suggested remediation

Not verified input

After seeing that the first few are validated, I'll just mark this as invalid

Treasury may be address(0)

Bruh

GalloDaSballo · 2022-06-22T14:49:29Z

After checking a bunch more lines

Solidity compiler versions mismatch
Hardcoded WETH
Init frontrun

etc

Am marking this as invalid as spam submission, check the bot robee

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 29, 2022

code423n4 added a commit that referenced this issue May 29, 2022

robee issue #13

49d9d86

chase-manning added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) labels Jun 6, 2022

GalloDaSballo added the invalid This doesn't seem right label Jun 22, 2022

JeeberC4 closed this as completed Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #13

QA Report #13

code423n4 commented May 29, 2022

GalloDaSballo commented Jun 21, 2022

GalloDaSballo commented Jun 22, 2022

QA Report #13

QA Report #13

Comments

code423n4 commented May 29, 2022

Mult instead div in compares

Code instance:

Missing fee parameter validation

Code instances:

Does not validate the input fee parameter

Code instances:

safeApprove of openZeppelin is deprecated

Code instances:

Require with not comprehensive message

Code instance:

Not verified input

Code instances:

Treasury may be address(0)

Code instance:

Solidity compiler versions mismatch

Code instance:

Hardcoded WETH

Code instances:

Not verified owner

Code instance:

Init frontrun

Code instances:

Named return issue

Code instances:

Two Steps Verification before Transferring Ownership

Code instances:

In the following public update functions no value is returned

Code instance:

Never used parameters

Code instances:

Missing commenting

Code instance:

Check transfer receiver is not 0 to avoid burned money

Code instances:

In the following public update functions no value is returned

Code instance:

Never used parameters

Code instances:

Missing commenting

Code instance:

Add a timelock

Code instances:

GalloDaSballo commented Jun 21, 2022

Mult instead div in compares

Missing fee parameter validation

safeApprove of openZeppelin is deprecated

Require with not comprehensive message

Not verified input

Treasury may be address(0)

GalloDaSballo commented Jun 22, 2022