QA Report

[code423n4]

Table of Contents:

• [L-01] Add constructor initializers
• [L-02] Deprecated safeApprove() function
• [N-01] Open TODOS
• [N-02] Deprecated library used for Solidity 0.8.+ SafeMath
• [N-03] Unused named returns

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

contracts-full/CrossChainCanonicalAlchemicTokenV2.sol:
   8:   function initialize(
   9        string memory name, 
  10        string memory symbol, 
  11        address[] memory _bridgeTokens
  12:   ) public initializer {

contracts-full/CrossChainCanonicalGALCX.sol:
   7:   function initialize(
   8        string memory name, 
   9        string memory symbol, 
  10        address[] memory _bridgeTokens
  11:   ) public initializer {

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

contracts-full/AlchemistV2.sol:
  382:         TokenUtils.safeApprove(yieldToken, config.adapter, type(uint256).max);
  383:         TokenUtils.safeApprove(adapter.underlyingToken(), config.adapter, type(uint256).max);
  478:         TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
  479:         TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max);

contracts-full/EthAssetManager.sol:
  576:             SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0);
  577:             SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]);
  620:             SafeERC20.safeApprove(address(token), address(metaPool), 0);
  621:             SafeERC20.safeApprove(address(token), address(metaPool), amount);
  671:         SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0);
  672:         SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount);

contracts-full/ThreePoolAssetManager.sol:
  782:             SafeERC20.safeApprove(address(tokens[i]), address(threePool), 0);
  783:             SafeERC20.safeApprove(address(tokens[i]), address(threePool), amounts[i]);
  838:         SafeERC20.safeApprove(address(token), address(threePool), 0);
  839:         SafeERC20.safeApprove(address(token), address(threePool), amount);
  879:         SafeERC20.safeApprove(address(threePoolToken), address(threePool), 0);
  880:         SafeERC20.safeApprove(address(threePoolToken), address(threePool), amount);
  908:             SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0);
  909:             SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]);
  944:         SafeERC20.safeApprove(address(token), address(metaPool), 0);
  945:         SafeERC20.safeApprove(address(token), address(metaPool), amount);
  987:         SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0);
  988:         SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount);

contracts-full/TransmuterBuffer.sol:
  236:                 TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, 0); ////
  238:             TokenUtils.safeApprove(debtToken, alchemist, 0); ////
  243:             TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, type(uint256).max); ////
  245:         TokenUtils.safeApprove(debtToken, alchemist, type(uint256).max); ////
  284:         TokenUtils.safeApprove(underlyingToken, alchemist, type(uint256).max); ////

contracts-full/adapters/fuse/FuseTokenAdapterV1.sol:
  71:         SafeERC20.safeApprove(underlyingToken, token, amount);

contracts-full/adapters/lido/WstETHAdapterV1.sol:
  105:         SafeERC20.safeApprove(parentToken, address(token), mintedStEth);
  129:         SafeERC20.safeApprove(parentToken, curvePool, unwrappedStEth);

contracts-full/adapters/vesper/VesperAdapterV1.sol:
  62:         SafeERC20.safeApprove(underlyingToken, token, amount);

contracts-full/adapters/yearn/YearnTokenAdapter.sol:
  32:         TokenUtils.safeApprove(underlyingToken, token, amount);

[N-01] Open TODOS

Consider resolving the TODOs before deploying.

File: IStableMetaPool.sol
6: /// @dev TODO
7: uint256 constant N_COINS = 2;

[N-02] Deprecated library used for Solidity 0.8.+ SafeMath

contracts-full/TransmuterBuffer.sol:
   7: import "@openzeppelin/contracts/utils/math/SafeMath.sol";
  27:     using SafeMath for uint256;

[N-03] Unused named returns

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

contracts-full/EthAssetManager.sol:
  367:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  380:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  393:     ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns
  404:     ) external lock onlyOperator returns (bool success) { //@audit unused named returns
  415:     ) external lock onlyOperator returns (bool success) { //@audit unused named returns
  521:     ) external lock onlyAdmin returns (bytes memory result) { //@audit unused named returns

contracts-full/ThreePoolAssetManager.sol:
  334:     ) public view returns (uint256 delta, bool add) { //@audit unused named returns
  534:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  547:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  560:     ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns
  571:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  584:     ) external lock onlyOperator returns (uint256 minted) { //@audit unused named returns
  597:     ) external lock onlyOperator returns (uint256 withdrawn) { //@audit unused named returns
  608:     ) external lock onlyOperator returns (bool success) { //@audit unused named returns
  619:     ) external lock onlyOperator returns (bool success) { //@audit unused named returns

contracts-full/TransmuterBuffer.sol:
  134:         returns (uint256 weight) //@audit unused named returns

contracts-full/TransmuterV2.sol:
  350:   function getUnexchangedBalance(address owner) external view override returns (uint256 unexchangedBalance) { //@audit unused named returns
  370:   function getExchangedBalance(address owner) external view override returns (uint256 exchangedBalance) { //@audit unused named returns
  374:   function getClaimableBalance(address owner) external view override returns (uint256 claimableBalance) { //@audit unused named returns
  554:   function _getExchangedBalance(address owner) internal view returns (uint256 exchangedBalance) { //@audit unused named returns

[0xfoobar]

Good QA