QA Report

[code423n4]

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.

[0xfoobar]

Sponsor acknowledged

Alchemix does not deal with fee-on-transfer or rebasing tokens

[0xleastwood]

As Alchemix does not deal with fee-on-transfer tokens, I'm inclined to mark this as QA because it assumes the protocol governance enlists a token type that is not compatible with the protocol's design.