Alchemix contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemicTokenV2Base.sol#L190

Vulnerability details

AlchemicTokenV2Base's lowerHasMinted() function has onlyWhitelisted access control. Any minter is whitelisted, it is required to be able to run mint(). Each minters' mint total amount is controlled by totalMinted cumulative counter, which can be reduced by running lowerHasMinted().

This way any minter can arbitrary lower mint limit for herself, so mint limit can be surpassed, it actually controls only a single mint amount. Even small mintCeiling allows for unlimited mint.

Setting severity to high as this is a direct violation of system logic propagated to any contract that inherits from AlchemicTokenV2Base (it's CrossChainCanonicalAlchemicTokenV2 in the scope).

Proof of Concept

Any minter is whitelisted as it's required in order to use mint():

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemicTokenV2Base.sol#L103-L119

  /// @notice Mints tokens to `a recipient.`
  ///
  /// @notice This function reverts if `msg.sender` is not whitelisted.
  /// @notice This function reverts if `msg.sender` is paused.
  /// @notice This function reverts if `msg.sender` has exceeded their mintable ceiling.
  ///
  /// @param recipient The address to mint the tokens to.
  /// @param amount    The amount of tokens to mint.
  function mint(address recipient, uint256 amount) external onlyWhitelisted {
    if (paused[msg.sender]) {
      revert IllegalState();
    }

    uint256 total = amount + totalMinted[msg.sender];
    if (total > mintCeiling[msg.sender]) {
      revert IllegalState();
    }

Any whitelisted, and this way any minter, can run lowerHasMinted function, arbitrary lowering totalMinted:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemicTokenV2Base.sol#L184-L191

  /// @notice Lowers the number of tokens which the `msg.sender` has minted.
  ///
  /// @notice This reverts if the `msg.sender` is not whitelisted.
  ///
  /// @param amount The amount to lower the minted amount by.
  function lowerHasMinted(uint256 amount) external onlyWhitelisted {
    totalMinted[msg.sender] = totalMinted[msg.sender] - amount;
  }

This way totalMinted can be kept to be zero by any minter, and mintCeiling can only control one mint() amount instead of the total. Even small mintCeiling allows for unlimited mint by repeating mint -> lowerHasMinted call sequence.

I.e. mint amount limitation can be surpassed by any minter, the limit is effectively rendered void.

Recommended Mitigation Steps

Consider making lowerHasMinted onlySentinel or onlyAdmin.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L896-L905 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/EthAssetManager.sol#L566-L573

Vulnerability details

Both contracts treat meta assets as if they have fixed decimals of 18. Minting logic breaks when it's not the case. However, meta tokens decimals aren't controlled.

If actual meta assets have any other decimals, minting slippage control logic of both contracts will break up as total is calculated as a plain sum of token amounts.

In the higher token decimals case minTotalAmount will be magnitudes higher than actual amount Curve can provide and minting becomes unavailable.

In the lower token decimals case minTotalAmount will lack value and slippage control will be rendered void, which opens up a possibility of a fund loss from the excess slippage.

Setting severity to medium as the contract can be used with various meta tokens (_metaPoolAssetCache can be filled with any assets) and, whenever decimals differ from 18 add_liquidity uses, its logic be broken: the inability to mint violates the contract purpose, the lack of slippage control can lead to fund losses.

I.e. this is system breaking impact conditional on a low probability assumption of different meta token decimals.

Proof of Concept

Meta tokens decimals are de facto hard coded into the contract as plain amounts are used (L. 905):

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L896-L905

    function _mintMetaPoolTokens(
        uint256[NUM_META_COINS] calldata amounts
    ) internal returns (uint256 minted) {
        IERC20[NUM_META_COINS] memory tokens = _metaPoolAssetCache;

        uint256 total = 0;
        for (uint256 i = 0; i < NUM_META_COINS; i++) {
            if (amounts[i] == 0) continue;

            total += amounts[i];

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L915-L919

        uint256 expectedOutput    = total * CURVE_PRECISION / metaPool.get_virtual_price();
        uint256 minimumMintAmount = expectedOutput * metaPoolSlippage / SLIPPAGE_PRECISION;

        // Add the liquidity to the pool.
        minted = metaPool.add_liquidity(amounts, minimumMintAmount);

The same plain sum approach is used in EthAssetManager._mintMetaPoolTokens:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/EthAssetManager.sol#L566-L573

        uint256 total = 0;
        for (uint256 i = 0; i < NUM_META_COINS; i++) {
            // Skip over approving WETH since we are directly swapping ETH.
            if (i == uint256(MetaPoolAsset.ETH)) continue;

            if (amounts[i] == 0) continue;

            total += amounts[i];

When this decimals assumption doesn't hold, the slippage logic will not hold too: either the mint be blocked or slippage control disabled.

Notice, that ThreePoolAssetManager.calculateRebalance do query alUSD decimals (which is inconsistent with the above as it’s either fix and control on inception or do not fix and accommodate the logic):

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L338-L338

decimals     = SafeERC20.expectDecimals(address(alUSD));

Recommended Mitigation Steps

If meta assets are always supposed to have fixed decimals of 18, consider controlling it at the construction time.

I.e. the decimals can be controlled in constructors:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/EthAssetManager.sol#L214-L219

        for (uint256 i = 0; i < NUM_META_COINS; i++) {
            _metaPoolAssetCache[i] = params.metaPool.coins(i);
            if (_metaPoolAssetCache[i] == IERC20(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE)) {
                _metaPoolAssetCache[i] = weth;
+           } else {
+           	// check the decimals
				}
        }

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L254-L256

        for (uint256 i = 0; i < NUM_META_COINS; i++) {
            _metaPoolAssetCache[i] = params.metaPool.coins(i);
+           // check the decimals            
        }

In this case further decimals reading as it's done in calculateRebalance() is redundant.

Otherwise (which is less recommended as fixed decimals assumption is viable and simplify the logic) the meta token decimals can be added to calculations similarly to stables:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L779-L779

normalizedTotal += amounts[i] * 10**missingDecimals;

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L30-L32

Vulnerability details

Some tokens do not allow for approval of positive amount when allowance is positive already (to handle approval race condition, most known example is USDT).

This can cause the function to stuck whenever a combination of such a token and leftover approval be met. The latter can be possible if, for example, yearn vault is becoming full on a particular wrap() call and accepts only a part of amount, not utilizing the approval fully.

Then each next safeApprove will revert and wrap becomes permanently unavailable. Setting the severity to medium as depositing (wrapping) is core functionality for the contract and its availability is affected.

Proof of Concept

wrap use one step approve:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L30-L32

    function wrap(uint256 amount, address recipient) external override returns (uint256) {
        TokenUtils.safeTransferFrom(underlyingToken, msg.sender, address(this), amount);
        TokenUtils.safeApprove(underlyingToken, token, amount);

Some ERC20 forbid the approval of positive amount when the allowance is positive:

https://github.com/d-xo/weird-erc20#approval-race-protections

For example, USDT is supported by Yearn and can be the underlying asset:

https://yearn.finance/#/vault/0x7Da96a3891Add058AdA2E826306D812C638D87a7

Recommended Mitigation Steps

As the most general approach consider approving zero before doing so for the amount:

    function wrap(uint256 amount, address recipient) external override returns (uint256) {
        TokenUtils.safeTransferFrom(underlyingToken, msg.sender, address(this), amount);
+      TokenUtils.safeApprove(underlyingToken, token, 0);
        TokenUtils.safeApprove(underlyingToken, token, amount);

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L230-L248

Vulnerability details

Currently setAlchemist doesn't check whether there are any open positions left with the old Alchemist before switching to the new one.

As this require a number of checks the probability of operational mistake isn't low and it's prudent to introduce the main controls directly to the code to minimize it. In the case if the system go on with new Alchemist before realizing that there are some funds left in the old one, tedious and error prone manual recovery will be needed. There is also going to be a corresponding reputational damage.

Setting the severity to medium as while the function is admin only, the impact is up to massive user fund freeze, i.e. this is system breaking with external assumptions.

Proof of Concept

Alchemist implementation change can happen while there are open deposits remaining with the current contract. As there looks to be no process to transfer them in the code, such TransmuterBuffer's funds will be frozen with old alchemist:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/TransmuterBuffer.sol#L230-L232

    function setAlchemist(address _alchemist) external override onlyAdmin {
        sources[alchemist] = false;
        sources[_alchemist] = true;

Recommended Mitigation Steps

Consider requiring that all exposure to the old Alchemist is closed, for example both getAvailableFlow and getTotalCredit is zero.

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L230-L231

    function setAlchemist(address _alchemist) external override onlyAdmin {
+		  require(getTotalCredit() == 0, "Credit exists with old Alchemist");
+       for (uint256 j = 0; j < registeredUnderlyings.length; j++) {
+           require(getTotalUnderlyingBuffered[registeredUnderlyings[j]] == 0, "Buffer exists with old Alchemist");
+       }
        sources[alchemist] = false;

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/gALCX.sol#L93-L94 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L69-L76

Vulnerability details

An attacker can become the first depositor for a recently created gALCX contract, providing a tiny amount of ALCX tokens by calling stake(1) (raw values here, 1 is 1 wei, 1e18 is 1 ALCX). Then the attacker can directly transfer, for example, 10^6 * 1e18 - 1 of ALCX to the gALCX contract and run bumpExchangeRate(), effectively setting the cost of 1 gALCX to be 10^6 * 1e18 of ALCX. The attacker will still own 100% of the gALCX's ALCX pool being the only depositor.

All subsequent depositors will have their ALCX token investments rounded to 10^6 * 1e18, due to the lack of precision which initial tiny deposit caused, with the remainder divided between all current depositors, i.e. the subsequent depositors lose value to the attacker.

For example, if the second depositor brings in 1.9*10^6 * 1e18 of ALCX, only 1 of new vault to be issued as 1.9*10^6 * 1e18 divided by 10^6 * 1e18 will yield just 1, which means that 2.9*10^6 * 1e18 total ALCX pool will be divided 50/50 between the second depositor and the attacker, as each will have 1 wei of the total 2 wei of vault tokens, i.e. the depositor lost and the attacker gained 0.45 * 10^6 * 1e18 of ALCX tokens.

As there are no penalties to exit with gALCX.unstake(), the attacker can remain staked for an arbitrary time, gathering the share of all new deposits' remainder amounts.

Placing severity to be medium as this is principal funds loss scenario for many users (most of depositors), easily executable, but only new gALCX contract instances are vulnerable.

Proof of Concept

gAmount of gALCX to be minted is determined as a quotient of amount provided and exchangeRate:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/gALCX.sol#L93-L94

        uint gAmount = amount * exchangeRatePrecision / exchangeRate;
        _mint(msg.sender, gAmount);

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L15

uint public constant exchangeRatePrecision = 1e18;

exchangeRate accumulates balance increments relative to total gALCX supply:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L69-L76

    function bumpExchangeRate() public {
        // Claim from pool
        pools.claim(poolId);
        // Bump exchange rate
        uint balance = alcx.balanceOf(address(this));

        if (balance > 0) {
            exchangeRate += (balance * exchangeRatePrecision) / totalSupply;

When gALCX contract is new, the very first stake -> bumpExchangeRate() yields nothing as the balance is empty, i.e. exchangeRate is still 1e18 and gAmount == amount:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L85-L93

    function stake(uint amount) external {
        // Get current exchange rate between ALCX and gALCX
        bumpExchangeRate();
        // Then receive new deposits
        bool success = alcx.transferFrom(msg.sender, address(this), amount);
        require(success, "Transfer failed");
        pools.deposit(poolId, amount);
        // gAmount always <= amount
        uint gAmount = amount * exchangeRatePrecision / exchangeRate;

This way, as there is no minimum amount or special treatment for the first deposit, the very first gAmount can be made 1 wei with stake(1) call.

Then, a combination of direct ALCX transfer and bumpExchangeRate() will make exchangeRate equal to the total amount provided by the attacker, say 10^6 * 1e18 * 1e18, as totalSupply is 1 wei.

When a second depositor enters, the amount of gALCX to be minted is calculated as amount * exchangeRatePrecision / exchangeRate, which is amount / (10^6 * 1e18), which will trunk the amount to the nearest divisor of 10^6 * 1e18, effectively dividing the remainder between the depositor and the attacker.

For example, if the second depositor brings in 1.9*10^6 ALCX, only 1 (1 wei) of gALCX to be issued as 1.9*10^6 * 1e18 * 1e18 / (10^6 * 1e18 * 1e18) = 1.

As the attacker and depositor both have 1 of gALCX, each owns (2.9 / 2)*10^6 * 1e18 = 1.45*10^6 * 1e18, so the attacker effectively stole 0.45*10^6 * 1e18 from the depositor.

Any deposit lower than total attacker's stake, 10^6 * 1e18, will be fully stolen from the depositor as 0 gALCX tokens will be issued in this case.

References

The issue is similar to the TOB-YEARN-003 one of the Trail of Bits audit of Yearn Finance:

https://github.com/yearn/yearn-security/tree/master/audits/20210719_ToB_yearn_vaultsv2

Recommended Mitigation Steps

A minimum for deposit value can drastically reduce the economic viability of the attack. I.e. stake() can require each amount to surpass the threshold, and then an attacker would have to provide too big direct investment to capture any meaningful share of the subsequent deposits.

An alternative is to require only the first depositor to freeze big enough initial amount of liquidity. This approach has been used long enough by various projects, for example in Uniswap V2:

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L377-L379

Vulnerability details

TransmuterBuffer's refreshStrategies() is the only way to actualize _yieldTokens array. The function requires registeredUnderlyings array to match current Alchemist's _supportedUnderlyingTokens. In the same time registeredUnderlyings can be only increased via registerAsset(): there is no way to reduce, remove or reconstruct the array.

This way if registerAsset() was mistakenly called extra time or alchemist was switched with setAlchemist to a new one with less supported assets, then the strategy refresh becomes impossible and the TransmuterBuffer be blocked as it cannot be properly used without synchronization with Alchemist.

The redeployment of the contract doesn't provide an easy fix as it holds the accounting data that needs to be recreated (flowAvailable, currentExchanged mappings).

Proof of Concept

refreshStrategies require registeredUnderlyings to be equal to Alchemist's supportedUnderlyingTokens:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L377-L379

        if (registeredUnderlyings.length != supportedUnderlyingTokens.length) {
            revert IllegalState();
        }

If registeredUnderlyings has length more than Alchemist's _supportedUnderlyingTokens it doesn't look to be fixable and prohibits the future use of the contract, i.e. breaks the system.

Recommended Mitigation Steps

The issue is that there is no way to unregister the asset, so consider introducing a function to remove the underlying or simply delete the array so it can be reconstructed with a sequence of registerAsset calls.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/TransmuterBuffer.sol#L511-L524

Vulnerability details

exchange() -> _exchange() -> _alchemistWithdraw() is user funds utilizing call sequence and the slippage hard coded to 1% there can cause a range of issues.

For example, if there is not enough shares, the number of shares to withdraw will be unconditionally reduced to the number of the shares available. This can pass under 1% slippage and user will give away up to 1% without giving a consent to such a fee, which is big enough to notice.

On the other hand, in a similar situation when there is not enough shares available a user might knowingly want to execute with even bigger fee, but hard coded slippage will not be met and the withdraw be unavailable and funds frozen.

Setting the severity to medium as the end impact is either modest user fund loss or exchange functionality unavailability.

Proof of Concept

_alchemistWithdraw uses hard coded 1% slippage threshold and rewrites wantShares to be availableShares once TransmuterBuffer's position isn't big enough:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/TransmuterBuffer.sol#L511-L524

    function _alchemistWithdraw(address token, uint256 amountUnderlying) internal {
        uint8 decimals = TokenUtils.expectDecimals(token);
        uint256 pricePerShare = IAlchemistV2(alchemist).getUnderlyingTokensPerShare(token);
        uint256 wantShares = amountUnderlying * 10**decimals / pricePerShare;
        (uint256 availableShares, uint256 lastAccruedWeight) = IAlchemistV2(alchemist).positions(address(this), token);
        if (wantShares > availableShares) {
            wantShares = availableShares;
        }
        // Allow 1% slippage
        uint256 minimumAmountOut = amountUnderlying - amountUnderlying * 100 / 10000;
        if (wantShares > 0) {
            IAlchemistV2(alchemist).withdrawUnderlying(token, wantShares, address(this), minimumAmountOut);
        }
    }

Alchemist's _unwrap will revert withdrawUnderlying call once minimumAmountOut isn't met:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1344-L1346

        if (amountUnwrapped < minimumAmountOut) {
            revert SlippageExceeded(amountUnwrapped, minimumAmountOut);
        }

There are 2 use cases for the function:

exchange (onlyKeeper) -> _exchange -> _alchemistWithdraw,

setFlowRate (onlyAdmin) -> _exchange -> _alchemistWithdraw

exchange() is the most crucial as it should be able to fulfil various types of user funds exchange requests:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L526-L546

    /// @notice Pull necessary funds from the Alchemist and exchange them.
    ///
    /// @param underlyingToken The underlying-token to exchange.
    function _exchange(address underlyingToken) internal {
        _updateFlow(underlyingToken);

        uint256 totalUnderlyingBuffered = getTotalUnderlyingBuffered(underlyingToken);
        uint256 initialLocalBalance = TokenUtils.safeBalanceOf(underlyingToken, address(this));
        uint256 want = 0;
        // Here we assume the invariant underlyingToken.balanceOf(address(this)) >= currentExchanged[underlyingToken].
        if (totalUnderlyingBuffered < flowAvailable[underlyingToken]) {
            // Pull the rest of the funds from the Alchemist.
            want = totalUnderlyingBuffered - initialLocalBalance;
        } else if (initialLocalBalance < flowAvailable[underlyingToken]) {
            // totalUnderlyingBuffered > flowAvailable so we have funds available to pull.
            want = flowAvailable[underlyingToken] - initialLocalBalance;
        }

        if (want > 0) {
            _alchemistAction(want, underlyingToken, _alchemistWithdraw);
        }

This way, one issue here is that user can end up giving away the full 1% unconditionally to market situation because there are not enough shares available.

Another one is that knowing that the conditions are bad or that there are not enough shares available and willing to run the exchange with bigger slippage the user will not be able to as there are no means to control it and the functionality will end up unavailable, being reverted by Alchemist's _unwrap check.

Recommended Mitigation Steps

Consider adding the function argument with a default value of 1%, so the slippage can be tuned when it is needed.

1. AutoleverageCurve contracts use unsafe underlying token transfer (low)

AutoleverageCurveFactoryethpool._transferTokensToSelf:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AutoleverageCurveFactoryethpool.sol#L26-L26

IERC20(underlyingToken).transferFrom(msg.sender, address(this), collateralInitial);

AutoleverageCurveMetapool._transferTokensToSelf

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AutoleverageCurveMetapool.sol#L16-L16

IERC20(underlyingToken).transferFrom(msg.sender, address(this), collateralInitial);

Recommended Mitigation Steps

Require transfer success or consider safeTransfer as not all the tokens revert on failure, leading to internal discrepancies and funds freezing when it's unaccounted.

2. Old pool approval isn't removed by gALCX's migrateSource (low)

Old pool contract no longer used still has unlimited approval.

Proof of Concept

migrateSource only gives new approval to the new contract:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/gALCX.sol#L57

Old remain the same:

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L27

reApprove();

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/gALCX.sol#L62-L63

    function reApprove() public {
        bool success = alcx.approve(address(pools), type(uint).max);

Recommended Mitigation Steps

Consider setting to zero allowance for the old pool

3. Array lengths aren't checked to match (non-critical)

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterBuffer.sol#L178-L183

    function setWeights(
        address weightToken,
        address[] memory tokens,
        uint256[] memory weights
    ) external override onlyAdmin {
        Weighting storage weighting = weightings[weightToken];

4. Events aren't indexed (non-critical)

Filtering on unindexed events is disabled, which makes it harder to programmatically use and analyse the system.

Proof of Concept

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2Base.sol#L55

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L20-L21

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/ThreePoolAssetManager.sol#L69-L167

Recommended Mitigation Steps

Consider adding indexes to ids and addresses in the all important events to improve their usability

5. No core configuration zero address checks (non-critical)

Admin and token addresses not checked to be non-zero: https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L54

New bridge token address isn't checked to be non-zero https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/CrossChainCanonicalBase.sol#L148

Recommended Mitigation Steps

Add the checks to ensure correct configuration and reduce operational risks

6. Token address argument is redundant (non-critical)

Proof of Concept

Arguments aren't used:

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2Base.sol#L206

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2Base.sol#L219

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2.sol#L174

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/AlchemicTokenV2.sol#L187

Recommended Mitigation Steps

Consider removing both arguments, the contract reply for itself by default

7. Floating pragma is used across the system (non-critical)

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced.

Proof of Concept

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-hardhat/AlchemistV2.sol#L1-L1

pragma solidity ^0.8.11;

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/adapters/yearn/YearnTokenAdapter.sol#L1

pragma solidity ^0.8.11;

Recommended Mitigation Steps

Consider fixing the version for the whole set of system contracts.

8. gALCX ownership is transferred with one step procedure (non-critical)

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the ownership change.

Proof of Concept

gALCX owner is set immediately in one step:

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-hardhat/gALCX.sol#L39

Recommended Mitigation Steps

Consider utilizing two-step ownership transferring process (proposition and acceptance in the separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system.