Alchemix contest - ellahi's results

Low

[L-01] Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Proof of Concept

gALCX.sol#L39-L41

function transferOwnership(address _owner) external onlyOwner {
    owner = _owner;
}

Recommendation

Consider adding a pendingOwner where the new owner will have to accept the ownership.

address owner;
address pendingOwner;

// ...

function setPendingOwner(address newPendingOwner) external {
    require(msg.sender == owner, "!owner");
    emit NewPendingOwner(newPendingOwner);
    pendingOwner = newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner, "!pendingOwner");
    emit NewOwner(pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

[L-02] Missing checks for address(0) when assigning values to address state variables.

Impact

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

Proof of Concept

gALCX.sol#L39-L41

    function transferOwnership(address _owner) external onlyOwner { // @audit make it two-step
        owner = _owner;
    }

Recommendation

Add address(0) checks.

[L-03] Do not use Deprecated Library Functions

Impact

The usage of deprecated library functions, safeApprove & _setupRole in this case, should be discouraged.

Proof of Concept

  AlchemicTokenV1.sol::43 => _setupRole(ADMIN_ROLE, msg.sender);
  AlchemicTokenV1.sol::44 => _setupRole(SENTINEL_ROLE, msg.sender);
  AlchemicTokenV1.sol::102 => _setupRole(SENTINEL_ROLE, sentinel);
  AlchemicTokenV2.sol::56 => _setupRole(ADMIN_ROLE, msg.sender);
  AlchemicTokenV2.sol::57 => _setupRole(SENTINEL_ROLE, msg.sender);
  AlchemicTokenV2.sol::129 => _setupRole(SENTINEL_ROLE, sentinel);
  AlchemicTokenV2Base.sol::63 => _setupRole(ADMIN_ROLE, msg.sender);
  AlchemicTokenV2Base.sol::64 => _setupRole(SENTINEL_ROLE, msg.sender);
  AlchemicTokenV2Base.sol::142 => _setupRole(SENTINEL_ROLE, sentinel);
  AlchemistV2.sol::382 => TokenUtils.safeApprove(yieldToken, config.adapter, type(uint256).max);
  AlchemistV2.sol::383 => TokenUtils.safeApprove(adapter.underlyingToken(), config.adapter, type(uint256).max);
  AlchemistV2.sol::478 => TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
  AlchemistV2.sol::479 => TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max);
  EthAssetManager.sol::576 => SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0);
  EthAssetManager.sol::577 => SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]);
  EthAssetManager.sol::620 => SafeERC20.safeApprove(address(token), address(metaPool), 0);
  EthAssetManager.sol::621 => SafeERC20.safeApprove(address(token), address(metaPool), amount);
  EthAssetManager.sol::671 => SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0);
  EthAssetManager.sol::672 => SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount);
  ThreePoolAssetManager.sol::782 => SafeERC20.safeApprove(address(tokens[i]), address(threePool), 0);
  ThreePoolAssetManager.sol::783 => SafeERC20.safeApprove(address(tokens[i]), address(threePool), amounts[i]);
  ThreePoolAssetManager.sol::838 => SafeERC20.safeApprove(address(token), address(threePool), 0);
  ThreePoolAssetManager.sol::839 => SafeERC20.safeApprove(address(token), address(threePool), amount);
  ThreePoolAssetManager.sol::879 => SafeERC20.safeApprove(address(threePoolToken), address(threePool), 0);
  ThreePoolAssetManager.sol::880 => SafeERC20.safeApprove(address(threePoolToken), address(threePool), amount);
  ThreePoolAssetManager.sol::908 => SafeERC20.safeApprove(address(tokens[i]), address(metaPool), 0);
  ThreePoolAssetManager.sol::909 => SafeERC20.safeApprove(address(tokens[i]), address(metaPool), amounts[i]);
  ThreePoolAssetManager.sol::944 => SafeERC20.safeApprove(address(token), address(metaPool), 0);
  ThreePoolAssetManager.sol::945 => SafeERC20.safeApprove(address(token), address(metaPool), amount);
  ThreePoolAssetManager.sol::987 => SafeERC20.safeApprove(address(metaPool), address(convexBooster), 0);
  ThreePoolAssetManager.sol::988 => SafeERC20.safeApprove(address(metaPool), address(convexBooster), amount);
  TransmuterBuffer.sol::85 => _setupRole(ADMIN, _admin);
  TransmuterBuffer.sol::236 => TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, 0);
  TransmuterBuffer.sol::238 => TokenUtils.safeApprove(debtToken, alchemist, 0);
  TransmuterBuffer.sol::243 => TokenUtils.safeApprove(registeredUnderlyings[i], alchemist, type(uint256).max);
  TransmuterBuffer.sol::245 => TokenUtils.safeApprove(debtToken, alchemist, type(uint256).max);
  TransmuterBuffer.sol::284 => TokenUtils.safeApprove(underlyingToken, alchemist, type(uint256).max);
  TransmuterV2.sol::149 => _setupRole(ADMIN, msg.sender);
  adapters/fuse/FuseTokenAdapterV1.sol::71 => SafeERC20.safeApprove(underlyingToken, token, amount);
  adapters/lido/WstETHAdapterV1.sol::105 => SafeERC20.safeApprove(parentToken, address(token), mintedStEth);
  adapters/lido/WstETHAdapterV1.sol::129 => SafeERC20.safeApprove(parentToken, curvePool, unwrappedStEth);
  adapters/vesper/VesperAdapterV1.sol::62 => SafeERC20.safeApprove(underlyingToken, token, amount);
  adapters/yearn/YearnTokenAdapter.sol::32 => TokenUtils.safeApprove(underlyingToken, token, amount);

Recommendation

_setupRole function is deprecated in favor of _grantRole. Use safeIncreaseAllowance / safeDecreaseAllowance instead of safeApprove.

[L-04] Unsafe ERC20 Operations

Impact

The return value of an external transfer/transferFrom call is not checked

Proof of Concept

  AutoleverageCurveFactoryethpool.sol::26 => IERC20(underlyingToken).transferFrom(msg.sender, address(this), collateralInitial);
  AutoleverageCurveMetapool.sol::16 => IERC20(underlyingToken).transferFrom(msg.sender, address(this), collateralInitial);

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

[L-05] Incorrect comment.

TransmuterV2.sol#L182-L188

  /// @dev A modifier which checks if caller is a sentinel.
  modifier notPaused() {
    if (isPaused) {
      revert IllegalState();
    }
    _;
  }

The modifier does not check if caller is a sentinel. It checks if the function is paused or not.

Non-Critical

[N-01] Declare uint as uint256.

Recommendation

To favor explicitness, all instances of uint should be declared as uint256.

Tools used

manual, slither

Gas

[G-01] Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  AlchemistV2.sol::990 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1282 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1355 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1461 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1524 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  CrossChainCanonicalBase.sol::57 => for (uint256 i = 0; i < _bridgeTokens.length; i++){
  CrossChainCanonicalBase.sol::141 => for (uint i = 0; i < bridgeTokensArray.length; i++){
  StakingPools.sol::188 => for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
  StakingPools.sol::363 => for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
  TransmuterBuffer.sol::172 => for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) {
  TransmuterBuffer.sol::186 => for (uint256 i = 0; i < tokens.length; i++) {
  TransmuterBuffer.sol::235 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::242 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::272 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::382 => for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
  TransmuterBuffer.sol::479 => for (uint256 j = 0; j < weighting.tokens.length; j++) {
  base/Multicall.sol::14 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Store the array’s length in a variable before the for-loop.

Recommendation

Use != 0 instead of > 0.

[G-02] Don't initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

AlchemistV2.sol::990 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1282 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1355 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1458 => uint256 totalValue = 0;
  AlchemistV2.sol::1461 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1524 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  CrossChainCanonicalBase.sol::57 => for (uint256 i = 0; i < _bridgeTokens.length; i++){
  CrossChainCanonicalBase.sol::141 => for (uint i = 0; i < bridgeTokensArray.length; i++){
  EthAssetManager.sol::214 => for (uint256 i = 0; i < NUM_META_COINS; i++) {
  EthAssetManager.sol::566 => uint256 total = 0;
  EthAssetManager.sol::567 => for (uint256 i = 0; i < NUM_META_COINS; i++) {
  ThreePoolAssetManager.sol::250 => for (uint256 i = 0; i < NUM_STABLE_COINS; i++) {
  ThreePoolAssetManager.sol::254 => for (uint256 i = 0; i < NUM_META_COINS; i++) {
  ThreePoolAssetManager.sol::353 => for (uint256 i = 0; i < 256; i++) {
  ThreePoolAssetManager.sol::773 => for (uint256 i = 0; i < NUM_STABLE_COINS; i++) {
  ThreePoolAssetManager.sol::901 => uint256 total = 0;
  ThreePoolAssetManager.sol::902 => for (uint256 i = 0; i < NUM_META_COINS; i++) {
  TransmuterBuffer.sol::172 => for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) {
  TransmuterBuffer.sol::186 => for (uint256 i = 0; i < tokens.length; i++) {
  TransmuterBuffer.sol::235 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::242 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::272 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::382 => for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
  TransmuterBuffer.sol::387 => for (uint256 i = 0; i < numYTokens; i++) {
  TransmuterBuffer.sol::479 => for (uint256 j = 0; j < weighting.tokens.length; j++) {
  TransmuterBuffer.sol::534 => uint256 want = 0;
  TransmuterBuffer.sol::549 => uint256 exchangeDelta = 0;
  base/Multicall.sol::14 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Remove explicit zero initialization.

[G-03] ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Proof of Concept

  AlchemistV2.sol::990 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1282 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1355 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1461 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  AlchemistV2.sol::1524 => for (uint256 i = 0; i < depositedTokens.values.length; i++) {
  CrossChainCanonicalBase.sol::57 => for (uint256 i = 0; i < _bridgeTokens.length; i++){
  CrossChainCanonicalBase.sol::141 => for (uint i = 0; i < bridgeTokensArray.length; i++){
  StakingPools.sol::188 => for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
  StakingPools.sol::363 => for (uint256 _poolId = 0; _poolId < _pools.length(); _poolId++) {
  TransmuterBuffer.sol::172 => for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) {
  TransmuterBuffer.sol::186 => for (uint256 i = 0; i < tokens.length; i++) {
  TransmuterBuffer.sol::235 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::242 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::272 => for (uint256 i = 0; i < registeredUnderlyings.length; i++) {
  TransmuterBuffer.sol::382 => for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
  TransmuterBuffer.sol::479 => for (uint256 j = 0; j < weighting.tokens.length; j++) {
  base/Multicall.sol::14 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-04] Reduce the size of error messages (Long revert Strings).

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

  AlchemicTokenV1.sol::51 => require(whiteList[msg.sender], "AlTokenV1: Alchemist is not whitelisted");
  AlchemicTokenV1.sol::81 => require(total <= ceiling[msg.sender], "AlUSD: Alchemist's ceiling was breached.");
  StakingPools.sol::106 => require(_governance != address(0), "StakingPools: governance address cannot be 0x0");
  StakingPools.sol::124 => require(_pendingGovernance != address(0), "StakingPools: pending governance address cannot be 0x0");
  StakingPools.sol::131 => require(msg.sender == pendingGovernance, "StakingPools: only pending governance");
  StakingPools.sol::160 => require(tokenPoolIds[_token] == 0, "StakingPools: token already has a pool");
  StakingPools.sol::183 => require(_rewardWeights.length == _pools.length(), "StakingPools: weights length mismatch");

Recommendation

Shorten the revert strings to fit in 32 bytes, or use custom errors if >0.8.4.

[G-05] Redundant return.

Impact

Waste of gas. Check @audit.

Proof of Concept

TransmuterV2.sol#L554-L573

                                                         // @audit get rid of exchangedBalance
  function _getExchangedBalance(address owner) internal view returns (uint256 exchangedBalance) { 
    Account storage account = accounts[owner];
    // @audit add uint256 exchangedBalance; here
    if (account.occupiedTick <= satisfiedTick) {
      exchangedBalance = account.exchangedBalance; 
      exchangedBalance += account.unexchangedBalance; 
      return exchangedBalance; 
    }

    exchangedBalance = account.exchangedBalance;

    uint256 exchanged = LiquidityMath.calculateProduct(
      account.unexchangedBalance,
      ticks.getWeight(account.occupiedTick, ticks.position)
    );

    exchangedBalance += exchanged;  

    return exchangedBalance; 
  }

Recommendation

Get rid of named return and initialize variable in function. Saves a bit of gas.

Tools used

c4udit, manual, slither