Alchemix contest - horsefacts's results

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

Gas

Declare TransmuterConduit state variables immutable

The token, sourceTransmuter, and sinkTransmuter variables in TransmuterConduit are set permanently at construction time. They can be declared immutable to save runtime gas.

TransmuterConduit

    /// @notice The address of the underlying token that is being transmuted.
    address public token;

    /// @notice The address of the transmuter to pull funds from.
    address public sourceTransmuter;

    /// @notice The address of the transmuter to send funds to;.
    address public sinkTransmuter;

    constructor(address _token, address _source, address _sink) {
        token = _token;
        sourceTransmuter = _source;
        sinkTransmuter = _sink;
    }

Declare StakingPool reward token immutable

Similarly, the reward token in StakingPool can be declared immutable:

StakingPool

  /// @dev The token which will be minted as a reward for staking.
  IERC20Mintable public reward;