QA Report

[code423n4]

Table of Contents:

• [L-01] Unsafe casting may overflow
• [L-02] Add constructor initializers
• [L-03] Deprecated safeApprove() function
• [L-04] Deprecated approve() function
• [L-05] Lack of event emission after critical initialize() functions
• [L-06] No account existence check for low-level call
• [L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
• [N-01] Unused named returns

[L-01] Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

protocol/contracts/tokenomics/AmmConvexGauge.sol:
  208:         ammLastUpdated = uint48(block.timestamp);

protocol/contracts/tokenomics/AmmGauge.sol:
   41:         ammLastUpdated = uint48(block.timestamp);
  150:         ammLastUpdated = uint48(block.timestamp);

protocol/contracts/tokenomics/KeeperGauge.sol:
   49:         lastUpdated = uint48(block.timestamp);
  115:         lastUpdated = uint48(block.timestamp);

[L-02] 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:

• File: AddressProvider.sol

47:     constructor(address treasury) {
48:         AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, false);
49:         _addressKeyMetas.set(AddressProviderKeys._TREASURY_KEY, meta.toUInt());
50:         _setConfig(AddressProviderKeys._TREASURY_KEY, treasury);
51:     }
52: 
53:     function initialize(address roleManager) external initializer {
54:         AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, true);
55:         _addressKeyMetas.set(AddressProviderKeys._ROLE_MANAGER_KEY, meta.toUInt());
56:         _setConfig(AddressProviderKeys._ROLE_MANAGER_KEY, roleManager);
57:     }

• File: LpToken.sol

26:     constructor() ERC20Upgradeable() {}
27: 
28:     function initialize(
29:         string calldata name_,
30:         string calldata symbol_,
31:         uint8 decimals_,
32:         address _minter
33:     ) external override initializer returns (bool) {
34:         require(_minter != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
35:         __ERC20_init(name_, symbol_);
36:         _decimals = decimals_;
37:         minter = _minter;
38:         return true;
39:     }

• File: StakerVault.sol

61:     constructor(IController _controller)
62:         Authorization(_controller.addressProvider().getRoleManager())
63:     {
64:         controller = _controller;
65:         IInflationManager inflationManager_ = controller.inflationManager();
66:         require(address(inflationManager_) != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
67:         inflationManager = inflationManager_;
68:         addressProvider = _controller.addressProvider();
69:     }
70: 
71:     function initialize(address _token) external override initializer {
72:         token = _token;
73:     }

[L-03] 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:

protocol/contracts/CvxCrvRewardsLocker.sol:
  57:         IERC20(CRV).safeApprove(CRV_DEPOSITOR, type(uint256).max);
  60:         IERC20(CVX_CRV).safeApprove(CVX_CRV_STAKING, type(uint256).max);
  63:         IERC20(CRV).safeApprove(CVX_CRV_CRV_CURVE_POOL, type(uint256).max);
  66:         IERC20(CVX).safeApprove(CVX_LOCKER, type(uint256).max);

protocol/contracts/RewardHandler.sol:
  52:         IERC20(targetLpToken).safeApprove(address(bkdLocker), burnedAmount);
  64:         IERC20(token).safeApprove(spender, type(uint256).max);

protocol/contracts/tokenomics/AmmConvexGauge.sol:
  61:         IERC20(ammToken).safeApprove(booster, type(uint256).max);

protocol/contracts/tokenomics/FeeBurner.sol:
  118:         IERC20(token_).safeApprove(spender_, type(uint256).max);

protocol/contracts/zaps/PoolMigrationZap.sol:
  27:             IERC20(underlying_).safeApprove(address(newPool_), type(uint256).max);

[L-04] Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack and failing for certain token implementations that do not return a boolean value. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

File: VestedEscrow.sol
24:     constructor(address rewardToken_) {
25:         IERC20(rewardToken_).approve(msg.sender, type(uint256).max);
26:     }

[L-05] Lack of event emission after critical initialize() functions

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

• File: AddressProvider.sol

53:     function initialize(address roleManager) external initializer {
54:         AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, true);
55:         _addressKeyMetas.set(AddressProviderKeys._ROLE_MANAGER_KEY, meta.toUInt());
56:         _setConfig(AddressProviderKeys._ROLE_MANAGER_KEY, roleManager);
57:     }

• File: BkdLocker.sol

53:     function initialize(
54:         uint256 startBoost,
55:         uint256 maxBoost,
56:         uint256 increasePeriod,
57:         uint256 withdrawDelay
58:     ) external override onlyGovernance {
59:         require(currentUInts256[_START_BOOST] == 0, Error.CONTRACT_INITIALIZED);
60:         _setConfig(_START_BOOST, startBoost);
61:         _setConfig(_MAX_BOOST, maxBoost);
62:         _setConfig(_INCREASE_PERIOD, increasePeriod);
63:         _setConfig(_WITHDRAW_DELAY, withdrawDelay);
64:     }

• File: LpToken.sol

28:     function initialize(
29:         string calldata name_,
30:         string calldata symbol_,
31:         uint8 decimals_,
32:         address _minter
33:     ) external override initializer returns (bool) {
34:         require(_minter != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
35:         __ERC20_init(name_, symbol_);
36:         _decimals = decimals_;
37:         minter = _minter;
38:         return true;
39:     }

• File: StakerVault.sol

71:     function initialize(address _token) external override initializer {
72:         token = _token;
73:     }

[L-06] No account existence check for low-level call

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling.

Consider checking for account-existence before the call() to make this safely extendable to user-controlled address contexts in future (or, at least, prevent the address(0) entry):

File: GasBank.sol
67:     function withdrawFrom(
68:         address account,
69:         address payable to,
70:         uint256 amount
71:     ) public override {
72:         uint256 currentBalance = _balances[account];
73:         require(currentBalance >= amount, Error.NOT_ENOUGH_FUNDS);
74:         require(
75:             msg.sender == account || addressProvider.isAction(msg.sender),
76:             Error.UNAUTHORIZED_ACCESS
77:         );
78: 
79:         if (msg.sender == account) {
80:             uint256 ethRequired = controller.getTotalEthRequiredForGas(account);
81:             require(currentBalance - amount >= ethRequired, Error.NOT_ENOUGH_FUNDS);
82:         }
83:         _withdrawFrom(account, to, amount, currentBalance);
84:     }
85: 
86:     function _withdrawFrom(
87:         address account,
88:         address payable to,
89:         uint256 amount,
90:         uint256 currentBalance
91:     ) internal {
92:         _balances[account] = currentBalance.uncheckedSub(amount);
93: 
94:         // solhint-disable-next-line avoid-low-level-calls
95:         (bool success, ) = to.call{value: amount}(""); //@audit can be address(0)
96:         require(success, Error.FAILED_TRANSFER);
97: 
98:         emit Withdraw(account, to, amount);
99:     }

[L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

protocol/contracts/tokenomics/InflationManager.sol:
  627      function _getKeeperGaugeKey(address pool) internal pure returns (bytes32) {
  628:         return keccak256(abi.encodePacked(_KEEPER_WEIGHT_KEY, pool));
  629      }

  631      function _getAmmGaugeKey(address token) internal pure returns (bytes32) {
  632:         return keccak256(abi.encodePacked(_AMM_WEIGHT_KEY, token));
  633      }

  635      function _getLpStakerVaultKey(address vault) internal pure returns (bytes32) {
  636:         return keccak256(abi.encodePacked(_LP_WEIGHT_KEY, vault));
  637      }

[N-01] Unused named returns

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

tokenomics/FeeBurner.sol:47:        returns (uint256 received)
tokenomics/FeeBurner.sol:98:        returns (uint256 received)

[GalloDaSballo]

[L-01] Unsafe casting may overflow

Disagree as timestamp in seconds is well below that size and will be for the next few thousand years

[L-02] Add constructor initializers

Because the code doesn't seem upgradeable but uses initializers, I agree with adding the initializer in the constructor

[L-03] Deprecated safeApprove() function

Given the specific uses shown by the warden, I disagree that the code should be changed as all the instances can be proven to go from 0 to X allowance

[L-04] Deprecated approve() function

Idem + agree on using safe version

[L-05] Lack of event emission after critical initialize() functions

Disagree with this being low, per definition an event is an informational component

[L-06] No account existence check for low-level call

Can't remember if this was added in 0.8, will mark it as valid for now

[L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

This is a really interesting finding, personally believe that in the context of this system it will be fine

[N-01] Unused named returns

Agree

Pretty good report, well formatted, a few findings are very common and would benefit by being shorter but there were also a couple interesting ones