Alchemix contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function _normalizeUnderlyingTokensToDebt(address underlyingToken, uint256 amount) internal view returns (uint256) {
    return amount * _underlyingTokens[underlyingToken].conversionFactor;
}

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

function repay(address underlyingToken, uint256 amount, address recipient) external override lock returns (uint256) {
    ...
    uint256 credit = _normalizeUnderlyingTokensToDebt(underlyingToken, actualAmount);

    // Update the recipient's debt.
    _updateDebt(recipient, -SafeCast.toInt256(credit));
    ...

When repaying the debt with an underlyingToken, the amount in terms of the underlyingToken (adjusted for decimals) will always be taken in a 1:1 ratio/price for the subtrahend of the debt.

We believe this design is flawed and be exploited by arbitrageurs and eventually drives the market price of alToken to match the worst depegged underlyingToken.

Because if alToken is trading at a higher price against the depegged underlyingToken, the arbitrageur can always mint alToken and market sell for more depegged underlyingToken and repay the debt.

PoC

Given:

• alUSD is trading at $1
• minimumCollateralization: 4/3

When USDT is slightly depegged, and trading at $0.9:

An arbitrageur can:

1. Deposit 100M USDC as collateral and mint 75M alUSD (100*0.75);
2. Market buy 75M USDT with 67.5M alUSD (75*0.9) and repay the debt;
3. Withdraw the 100M USDC collateral;
4. Dump the remaining 7.5M alUSD (75-67.5) for profit.

This can be repeated until the market price of alUSD drops to $0.9, the same price as USDT.

Recommendation

Consider updating the repay() function and change to market buy using the underlyingToken to alToken and then burn() the alToken to reduce debt.

[L] Missing sanity check for the new adapter

The new adapter should be checked for a matching underlyingToken.

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

function setTokenAdapter(address yieldToken, address adapter) external override {
    _onlyAdmin();
    _checkState(yieldToken == ITokenAdapter(adapter).token());
    _checkSupportedYieldToken(yieldToken);
    _yieldTokens[yieldToken].adapter = adapter;
    TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
    TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max);
    emit TokenAdapterUpdated(yieldToken, adapter);
}

Recommendation

function setTokenAdapter(address yieldToken, address adapter) external override {
    _onlyAdmin();
    _checkState(yieldToken == ITokenAdapter(adapter).token());
    _checkState(_yieldTokens[yieldToken].underlyingToken == ITokenAdapter(adapter).underlyingToken());
    _checkSupportedYieldToken(yieldToken);
    _yieldTokens[yieldToken].adapter = adapter;
    TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
    TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max);
    emit TokenAdapterUpdated(yieldToken, adapter);
}

[L] Allowance granted to the old adapter should be revoked when setting the new adapter

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

function setTokenAdapter(address yieldToken, address adapter) external override {
    _onlyAdmin();
    _checkState(yieldToken == ITokenAdapter(adapter).token());
    _checkSupportedYieldToken(yieldToken);
    _yieldTokens[yieldToken].adapter = adapter;
    TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
    TokenUtils.safeApprove(ITokenAdapter(adapter).underlyingToken(), adapter, type(uint256).max);
    emit TokenAdapterUpdated(yieldToken, adapter);
}

Recommendation

function setTokenAdapter(address yieldToken, address adapter) external override {
    _onlyAdmin();
    _checkState(yieldToken == ITokenAdapter(adapter).token());
    _checkSupportedYieldToken(yieldToken);
    address oldAdapter = _yieldTokens[yieldToken].adapter;
    address underlyingToken = ITokenAdapter(adapter).underlyingToken();
    TokenUtils.safeApprove(yieldToken, oldAdapter, 0);
    TokenUtils.safeApprove(underlyingToken, oldAdapter, 0);
    _yieldTokens[yieldToken].adapter = adapter;
    TokenUtils.safeApprove(yieldToken, adapter, type(uint256).max);
    TokenUtils.safeApprove(underlyingToken, adapter, type(uint256).max);
    emit TokenAdapterUpdated(yieldToken, adapter);
}

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S-1] AlchemistV2.sol#_calculateUnrealizedActiveBalance() Implementation can be simpler and save some gas

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

        if (activeBalance == 0) {
          return activeBalance;
        }

Recommendation

Change to:

        if (activeBalance == 0) {
          return 0;
        }

[M-2] Avoid unnecessary storage writes can save gas

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

        YieldTokenParams storage yieldTokenParams = _yieldTokens[yieldToken];

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

        Account storage account = _accounts[owner];

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

        Limiters.LinearGrowthLimiter storage limiter = _repayLimiters[underlyingToken];

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

        Sets.AddressSet storage depositedTokens = _accounts[owner].depositedTokens;

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

        YieldTokenParams storage yieldTokenParams = _yieldTokens[yieldToken];

...

The gas cost of storage reads (SLOAD) is significant.

Copying to memory can save some gas.

[S-3] Cache array length in for loops can save gas

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.

Instances include:

• AlchemistV2.sol

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

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

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

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

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

• CrossChainCanonicalBase.sol

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

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

• StakingPools.sol

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

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

• TransmuterBuffer.sol

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

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

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

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

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

...

[M-4] Setting uint variables to 0 is redundant

Setting uint variables to 0 is redundant as they default to 0.

Instances include:

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

        uint256 totalValue = 0;

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

        uint256 total = 0;

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

        uint256 normalizedTotal   = 0;

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

        uint256 want = 0;

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

        uint256 exchangeDelta = 0;

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/test/yearn/YearnVaultMock.sol#L19

  uint256 public forcedSlippage = 0;

...

[S-5] Change unnecessary storage variables to constants can save gas

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/test/yearn/YearnVaultMock.sol#L11

  uint256 public min = 9500;

Some storage variables include min will not never be changed and they should not be.

Changing them to constant can save gas.

Recommendation

Change to:

uint256 public constant min = 9500;

[M-6] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

hhttps://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L990

        for (uint256 i = 0; i < depositedTokens.values.length; i++)

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

        for (uint256 i = 0; i < _bridgeTokens.length; i++)

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

        for (uint256 i = 0; i < NUM_META_COINS; i++) 

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

        for (uint256 i = 0; i < 256; i++)

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

        for (uint256 i = 0; i < _yieldTokens[underlyingToken].length; i++) 

...

[M-7] Unused local variables

Unused local variables in contracts increase contract size and gas usage at deployment.

Instances include:

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);
        }
    }

lastAccruedWeight is unused.

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

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

success is unused.

[S-8] Using immutable variable can save gas

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

    IERC20 public alcx = IERC20(0xdBdb4d16EdA451D0503b854CF79D55697F90c8DF);

Considering that alcx will never change, changing it to immutable variable instead of storage variable can save gas.

[S-9] Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

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

        if (cliff >= totalCliffs) return 0;

        uint256 reduction = totalCliffs - cliff;

totalCliffs - cliff will never underflow.

Recommendation

        if (cliff >= totalCliffs) return 0;
        unchecked{
            uint256 reduction = totalCliffs - cliff;
        }

[M-10] Avoid unnecessary arithmetic operations can save gas

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

        // Allow 1% slippage
        uint256 minimumAmountOut = amountUnderlying - amountUnderlying * 100 / 10000;

Recommendation

        // Allow 1% slippage
        uint256 minimumAmountOut = amountUnderlying * 9900 / 10000;

• reduce once subtraction

[M-11] Check if exchanged > 0 can save gas

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/TransmuterV2.sol#L417-L426

    if (state.unexchangedBalance > 0) {
      uint256 exchanged = LiquidityMath.calculateProduct(
        state.unexchangedBalance,
        ticks.getWeight(cache.occupiedTick, cache.currentTick)
      );

      state.totalUnexchanged -= exchanged;
      state.unexchangedBalance -= exchanged;
      state.exchangedBalance += exchanged;
    }

Recommendation

Change to:

    if (state.unexchangedBalance > 0) {
      uint256 exchanged = LiquidityMath.calculateProduct(
        state.unexchangedBalance,
        ticks.getWeight(cache.occupiedTick, cache.currentTick)
      );

    if (exchanged > 0){
        state.totalUnexchanged -= exchanged;
        state.unexchangedBalance -= exchanged;
        state.exchangedBalance += exchanged;
        }
    }

[M-12] Returning the named returns is redundant

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

  function _getExchangedBalance(address owner) internal view returns (uint256 exchangedBalance) {
    Account storage account = accounts[owner];

    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;
  }

At L572, returning exchangedBalance is redundant as named returns will be automatically handled.