Alchemix contest - AuditsAreUS's results

Lines of code

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

Vulnerability details

Impact

It is possible for any whitelisted used to reduce totalMinted for themselves. This value is used in mint() to prevent a malicious minter from minting an infinite number of tokens.

By allowing a minter to reduce their own totalMinted they are able to perform infinite minting.

Proof of Concept

The attack by a malicious minter is to call mint() then lowerHasMinted() in a loop to mint as many tokens as desired.

The minter first calls mint() which will update their totalMinted to amount and mint amount to an arbitrary receiver address. They minter may set amount = mintCeiling[msg.sender] to maximise the amount they may mint each loop/

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

    totalMinted[msg.sender] = total;

The malicious minter may then call lowerHasMinted to reduce totalMinted[msg.sender] = 0 for themselves.

  function lowerHasMinted(uint256 amount) external onlyWhitelisted {
    totalMinted[msg.sender] = totalMinted[msg.sender] - amount;
  }

The malicious minter may now call mint() again for mintCeiling[msg.sender] to mint more tokens for themselves. They then repeat calls to lowerHasMinted() and mint() until they have minted the desired number of tokens.

Recommended Mitigation Steps

There are two mitigations for this, the first is to remove the function lowerHasMinted(). The other option is to make lowerHasMinted() an adminOnly function preventing minters from adjusting their owner amount.

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/adapters/lido/WstETHAdapterV1.sol#L82-L84

Vulnerability details

Impact

The Lido adapter incorrectly calculates the price of WETH in terms of WstETH.

The function returns the price of WstETH in terms of stETH. The underlying token which we desire is WETH. Since stETH does not have the same value as WETH the output price incorrect.

The impact is severe as all the balance calculations require the price of the yield token converted to underlying. The incorrect price may over or understate the harvestable amount which is a core calculation in the protocol.

Proof of Concept

The function IWstETH(token).getStETHByWstETH() only converts WstETH to stETH. Thus, price() returns the value of WstETH in terms of stETH.

    function price() external view returns (uint256) {
        return IWstETH(token).getStETHByWstETH(10**SafeERC20.expectDecimals(token));
    }

Recommended Mitigation Steps

Add extra steps to price() to approximate the rate for converting stETH to ETH. This can be done using the same curve pool that is used to convert stETH to ETH in unwrap().

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1290-L1300 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1268 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1532 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L899 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1625

Vulnerability details

Impact

It is possible for the contract to become stuck and unable to perform any actions if the totalShares of a yield token fall to zero while there is some pendingCredit still to be paid.

It will then be impossible to call deposit or withdraw functions, mints, burns, repay, liquidate, donate or harvest due to division by zero reverts in:

• _distributeCredit()
• _distributeUnlockedCredit()
• _calculateUnrealizedDebt()
• _convertSharesToYieldTokens()
• donate()

Furthermore, any pendingCredit amount of tokens are still in the contract will become permanently stuck.

Proof of Concept

This case may arise under the follow steps a) deposit() is called by a user then time passes to earn some yield b) harvest() is called by the keeper which calls _distributeCredit() and increases pendingCredit c) withdraw() is called by the user to withdraw all funds

Since there is pendingCredit the following will have a non-zero balance for unlockedCredit however yieldTokenParams.totalShares is zero and thus we get a division by zero which reverts the entire transaction.

    function _distributeUnlockedCredit(address yieldToken) internal {
        YieldTokenParams storage yieldTokenParams = _yieldTokens[yieldToken];


        uint256 unlockedCredit = _calculateUnlockedCredit(yieldToken);
        if (unlockedCredit == 0) {
            return;
        }


        yieldTokenParams.accruedWeight     += unlockedCredit * FIXED_POINT_SCALAR / yieldTokenParams.totalShares;
        yieldTokenParams.distributedCredit += unlockedCredit;
    }

Each of the other listed functions will reach the same issue by attempting to divide some numerator by the totalShares which is zero.

Recommended Mitigation Steps

Consider preventing totalShares from over becoming zero once it is set. That is enforce a user to leave at least 1 unit if they are the last user to withdraw.

Another option is to transfer the first 1000 shares to a "burn" account (e.g. 0x000...01), when the first user deposits.

Alternatively, when the last user withdraws, transfer all pending credit to this user and set the required variables to zero to replicate the state before any users have deposited.

Fee on Transfer and Rebasing tokens are not accounted for in adapters or AlchemistV2

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/adapters/fuse/FuseTokenAdapterV1.sol#L66-L85 https://github.com/code-423n4/2022-05-alchemix/blob/de65c34c7b6e4e94662bf508e214dcbf327984f4/contracts-full/AlchemistV2.sol#L1319

Vulnerability details

Impact

Many ERC20 tokens are Fee on Transfer (FoT) in which a fee will be deducted from the amount transferred. The recipient will receive amount - fee tokens. Similarly rebasing tokens may change value up or down over time or during transfers.

If fee on transfer tokens are used in AlchemistV2 or the adapters the result is the contract may receive less tokens than sent in safeTranferFrom().

The impact of these tokens on the wrap() functions is that they will either a) spend the fee from the current contract balance b) revert if there is insufficient balance in the contract

Proof of Concept

AlchemistV2._wrap() will first transfer the tokens from the msg.sender to the current contract. It will then attempt to wrap this exact same amount of tokens. However if a fee has been applied during safeTransferFrom() then less than amount of tokens will be received by the contract and adapter.wrap() will either revert due to having insufficient balance or will spend the Alchemists tokens.

        TokenUtils.safeTransferFrom(underlyingToken, msg.sender, address(this), amount);
        uint256 wrappedShares = adapter.wrap(amount, address(this));
        if (wrappedShares < minimumAmountOut) {
            revert SlippageExceeded(wrappedShares, minimumAmountOut);
        }

FuseTokenAdapterV1.wrap() will transfer amount to this contract and then mint() the same amount in fuse. If they are FoT tokens the current contract will receive less than amount of tokens and therefore either mint() will fail due to insufficient balance or it will spend tokens from the contract which were not owned by the called.

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


        uint256 startingBalance = IERC20(token).balanceOf(address(this));


        uint256 error;
        if ((error = ICERC20(token).mint(amount)) != NO_ERROR) {
            revert FuseError(error);
        }


        uint256 endingBalance = IERC20(token).balanceOf(address(this));
        uint256 mintedAmount = endingBalance - startingBalance;


        SafeERC20.safeTransfer(token, recipient, mintedAmount);


        return mintedAmount;

The same principals also apply to YearnTokenAdpater.sol and VsperAdapterV1.sol.

Recommended Mitigation Steps

Ensure there is sufficient documentation such that underlying tokens which are FoT or rebasing are not added to the protocol as underlying tokens or yield tokens.