QA Report

[code423n4]

Low

WETH Gateway transfers out full contract balance

WethGateway#withdrawUnderlying unwraps and transfers the contract's full WETH balance to recipient:

WethGateway#withdrawUnderlying

    function withdrawUnderlying(
        address alchemist,
        address yieldToken,
        uint256 shares,
        address recipient,
        uint256 minimumAmountOut
    ) external {
        _onlyWhitelisted();
        // Ensure that the underlying of the target yield token is in fact WETH
        IAlchemistV2.YieldTokenParams memory params = IAlchemistV2(alchemist).getYieldTokenParameters(yieldToken);
        if (params.underlyingToken != address(WETH)) {
            revert IllegalArgument();
        }

        IAlchemistV2(alchemist).withdrawUnderlyingFrom(msg.sender, yieldToken, shares, address(this), minimumAmountOut);

        uint256 amount = WETH.balanceOf(address(this));
        WETH.withdraw(amount);

        (bool success, ) = recipient.call{value: amount}(new bytes(0));
        if (!success) {
            revert IllegalState();
        }
    }

If the gateway contract has an existing WETH balance, (for example, if a third party has accidentally or intentionally transferred WETH to the contract address), the caller may receive more ETH than expected.

Suggestion: use the return value of IAlchemistV2(alchemist).withdrawUnderlyingFrom.

Noncritical

Staking pools are incompatible with fee-on-transfer tokens

The internal accounting in StakingPools credits users with an amount equal to the _depositAmount argument:

[StakingPools#_deposit[(https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/StakingPools.sol#L375-L385)

  function _deposit(uint256 _poolId, uint256 _depositAmount) internal {
    Pool.Data storage _pool = _pools.get(_poolId);
    Stake.Data storage _stake = _stakes[msg.sender][_poolId];

    _pool.totalDeposited = _pool.totalDeposited + _depositAmount;
    _stake.totalDeposited = _stake.totalDeposited + _depositAmount;

    _pool.token.safeTransferFrom(msg.sender, address(this), _depositAmount);

    emit TokensDeposited(msg.sender, _poolId, _depositAmount);
  }

However, if the pool token is a fee-on-transfer ERC20, the amount received from safeTransferFrom on line 382 may be less than _depositAmount. This could impact reward accounting and potentially cause attempted withdrawals to revert for unlucky users.

Since the supported tokens are permissioned, this is a noncritical issue, but be aware of this incompatibility.

QA

Restrict _isLocked mutability to view

Mutex#_isLocked can be declared a view:

    function _isLocked() internal returns (bool) {
        return _lockState == 1;
    }

Missing address input validations

Address inputs are validated in most locations, but are not validated in a few places. Consider whether a zero or invalid address would impact these contracts.

• TransmuterBuffer#initialize

Unlocked version pragmas

It's a best practice to lock version pragmas for concrete contracts. Many contracts are using a fixed 0.8.13 pragma, but the following contracts have a floating ^0.8.11 pragma:

• AlchemicTokenV1
• AlchemicTokenV2
• AlchemicTokenV2Base
• AlchemistV2
• AutoleverageBase
• AutoleverageCurveFactoryethpool
• AutoleverageCurveMetapool
• CrossChainCanonicalAlchemicTokenV2
• CrossChainCanonicalBase
• CrossChainCanonicalGALCX
• gALCX
• StakingPools
• TransmuterBuffer
• TransmuterV2
• WETHGateway
• YearnTokenAdapter
• Multicall

Unused imports

• Math.sol in TransmuterBuffer
• LiquidityMath.sol in TransmuterBuffer

Incorrect comments

• Natspec for TransmuterV2#notPaused

Typos

• normalizedAmount in TransmuterV2#exchange
• for in comment for TransmuterBuffer#lastFlowrateUpdate

[0xleastwood]

I think the first point is useful to keep as is. Helps to ensure there is never locked ETH in the contract. If Ether is mistakenly sent to this contract, that is fair game.