TransmuterBuffer's _alchemistWithdraw use hard coded slippage that can lead to user losses

[code423n4]

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.

[thetechnocratic]

Sponsor acknowledged

Allowing for a caller-defined slippage would enable more flexibility when using the exchange() and setFlowRate() calls. However, the possibility of needing this flexibility at this time is very small, and because these functions are run by admins/keepers, there is room to modify the code if and when the flexibility becomes required.

[0xleastwood]

Agree with warden. During periods of high volatility, assets will be locked within the contract. As this limits protocol availability, potentially leading to further loss of funds as users cannot freely exit the protocol and sell tokens, medium risk is justified.