code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L440-L445 https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L932

Vulnerability details

Impact

The protocolFee is unlimited in AlchemistV2.sol, leading to maliciously moving large taxes from this contract.

Proof of Concept

There is almost no cap on protocolFee (You can set ProtocolFee equal to BPS to set 100% fee rate)

https://github.com/code-423n4/2022-05-alchemix/blob/main/contracts-full/AlchemistV2.sol#L440-L445

    function setProtocolFee(uint256 value) external override {
        _onlyAdmin();
        _checkArgument(value <= BPS);
        protocolFee = value;
        emit ProtocolFeeUpdated(value);
    }

When collecting protocol fee, unlimited protocolFee could lead to maliciously obtaining large amounts of tokens from users

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

    function harvest(address yieldToken, uint256 minimumAmountOut) external override lock {
        …
        uint256 feeAmount = amountUnderlyingTokens * protocolFee / BPS;
        uint256 distributeAmount = amountUnderlyingTokens - feeAmount;

        uint256 credit = _normalizeUnderlyingTokensToDebt(underlyingToken, distributeAmount);

        // Distribute credit to all of the users who hold shares of the yield token.
        _distributeCredit(yieldToken, credit);

        // Transfer the tokens to the fee receiver and transmuter.
        TokenUtils.safeTransfer(underlyingToken, protocolFeeReceiver, feeAmount);
        TokenUtils.safeTransfer(underlyingToken, transmuter, distributeAmount);

        // Inform the transmuter that it has received tokens.
        IERC20TokenReceiver(transmuter).onERC20Received(underlyingToken, distributeAmount);

        emit Harvest(yieldToken, minimumAmountOut, amountUnderlyingTokens);
    }

Tools Used

None

Recommended Mitigation Steps

There should be a reasonable cap on protocolFee. (e.g., 10%)

0xfoobar commented 2 years ago

Sponsor acknowledged

Assumed that governance acts in good faith.

0xleastwood commented 2 years ago

Governance is timelocked and held by multisig. Downgrading to QA.

Duplicate of #193.

code-423n4 / 2022-05-alchemix-findings