Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L50-L102 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L260-L272 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L275-L296 https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/internal/markets/Market.sol#L417-L450 https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/internal/markets/Market.sol#L746-L784 https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/external/actions/nTokenMintAction.sol#L266-L308

Vulnerability details

Impact

Single point of failure in NotionalV2 as it aren't audited with code4rena yet. It may expose some vulnerability to Flash loan attack.

Moreover, NotionalV2 may change in the future as shown in the walkthough video that some code still in a pull request. Changing code is the main source of vulnerability.

It may cause these vulnerable in WrappedFCash

Unlimited mint of WrappedFCash (_mintInternal)
User may withdraw malicious amount on maturity (_withdrawCashToAccount)
User may sell fCash in a malicious rate (_sellfCash)

Proof of Concept

_mintInternal

    function _mintInternal(
        uint256 depositAmountExternal,
        uint88 fCashAmount,
        address receiver,
        uint32 minImpliedRate,
        bool useUnderlying
    ) internal nonReentrant {
        require(!hasMatured(), "fCash matured");
        (IERC20 token, bool isETH) = getToken(useUnderlying);
        uint256 balanceBefore = isETH ? address(this).balance : token.balanceOf(address(this));

        // If dealing in ETH, we use WETH in the wrapper instead of ETH. NotionalV2 uses
        // ETH natively but due to pull payment requirements for batchLend, it does not support
        // ETH. batchLend only supports ERC20 tokens like cETH or aETH. Since the wrapper is a compatibility
        // layer, it will support WETH so integrators can deal solely in ERC20 tokens. Instead of using
        // "batchLend" we will use "batchBalanceActionWithTrades". The difference is that "batchLend"
        // is more gas efficient (does not require and additional redeem call to asset tokens). If using cETH
        // then everything will proceed via batchLend.
        if (isETH) {
            IERC20((address(WETH))).safeTransferFrom(msg.sender, address(this), depositAmountExternal);
            WETH.withdraw(depositAmountExternal);

            BalanceActionWithTrades[] memory action = EncodeDecode.encodeLendETHTrade(
                getCurrencyId(),
                getMarketIndex(),
                depositAmountExternal,
                fCashAmount,
                minImpliedRate
            );
            // Notional will return any residual ETH as the native token. When we _sendTokensToReceiver those
            // native ETH tokens will be wrapped back to WETH.
            NotionalV2.batchBalanceAndTradeAction{value: depositAmountExternal}(address(this), action);
        } else {
            // Transfers tokens in for lending, Notional will transfer from this contract.
            token.safeTransferFrom(msg.sender, address(this), depositAmountExternal);

            // Executes a lending action on Notional
            BatchLend[] memory action = EncodeDecode.encodeLendTrade(
                getCurrencyId(),
                getMarketIndex(),
                fCashAmount,
                minImpliedRate,
                useUnderlying
            );
            NotionalV2.batchLend(address(this), action);
        }

        // Mints ERC20 tokens for the receiver, the false flag denotes that we will not do an
        // operatorAck
        _mint(receiver, fCashAmount, "", "", false);

        _sendTokensToReceiver(token, msg.sender, isETH, balanceBefore);
    }

If hacker can bypass checks in NotionalV2.batchLend, hacker may passed arbitrary fCashAmount and got it minted via _mint.

_withdrawCashToAccount

    function _withdrawCashToAccount(
        uint16 currencyId,
        address receiver,
        uint88 assetInternalCashClaim,
        bool toUnderlying
    ) private returns (uint256 tokensTransferred) {
        (IERC20 token, bool isETH) = getToken(toUnderlying);
        uint256 balanceBefore = isETH ? address(this).balance : token.balanceOf(address(this));

        NotionalV2.withdraw(currencyId, assetInternalCashClaim, toUnderlying);

        tokensTransferred = _sendTokensToReceiver(token, receiver, isETH, balanceBefore);
    }

Flash loan attack to NotionalV2.withdraw may result in excessive cash withdrawal from NotionalV2.

_sellfCash

    function _sellfCash(
        address receiver,
        uint256 fCashToSell,
        bool toUnderlying,
        uint32 maxImpliedRate
    ) private returns (uint256 tokensTransferred) {
        (IERC20 token, bool isETH) = getToken(toUnderlying);
        uint256 balanceBefore = isETH ? address(this).balance : token.balanceOf(address(this));

        // Sells fCash on Notional AMM (via borrowing)
        BalanceActionWithTrades[] memory action = EncodeDecode.encodeBorrowTrade(
            getCurrencyId(),
            getMarketIndex(),
            _safeUint88(fCashToSell),
            maxImpliedRate,
            toUnderlying
        );
        NotionalV2.batchBalanceAndTradeAction(address(this), action);

        // Send borrowed cash back to receiver
        tokensTransferred = _sendTokensToReceiver(token, receiver, isETH, balanceBefore);
    }

Flash loan attack to NotionalV2.batchBalanceAndTradeAction may result in excessive token amount borrowed. And may caused many domino effects.

The highlighting parts of NotionalV2 that may vulnerable to Flash loan

https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/internal/markets/Market.sol#L417-L450

    function _getExchangeRate(
        int256 totalfCash,
        int256 totalCashUnderlying,
        int256 rateScalar,
        int256 rateAnchor,
        int256 fCashToAccount
    ) internal pure returns (int256, bool) {
        int256 numerator = totalfCash.subNoNeg(fCashToAccount);

        // This is the proportion scaled by Constants.RATE_PRECISION
        // (totalfCash + fCash) / (totalfCash + totalCashUnderlying)
        int256 proportion =
            numerator.divInRatePrecision(totalfCash.add(totalCashUnderlying));

        // This limit is here to prevent the market from reaching extremely high interest rates via an
        // excessively large proportion (high amounts of fCash relative to cash).
        // Market proportion can only increase via borrowing (fCash is added to the market and cash is
        // removed). Over time, the returns from asset cash will slightly decrease the proportion (the
        // value of cash underlying in the market must be monotonically increasing). Therefore it is not
        // possible for the proportion to go over max market proportion unless borrowing occurs.
        if (proportion > Constants.MAX_MARKET_PROPORTION) return (0, false);

        (int256 lnProportion, bool success) = _logProportion(proportion);
        if (!success) return (0, false);

        // lnProportion / rateScalar + rateAnchor
        int256 rate = lnProportion.divInRatePrecision(rateScalar).add(rateAnchor);
        // Do not succeed if interest rates fall below 1
        if (rate < Constants.RATE_PRECISION) {
            return (0, false);
        } else {
            return (rate, true);
        }
    }

This function may not working with large order that cause too much change to the rate. As shown in getfCashGivenCashAmount function.

https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/internal/markets/Market.sol#L746-L784

    function getfCashGivenCashAmount(
        int256 totalfCash,
        int256 netCashToAccount,
        int256 totalCashUnderlying,
        int256 rateScalar,
        int256 rateAnchor,
        int256 feeRate,
        int256 maxDelta
    ) internal pure returns (int256) {
        require(maxDelta >= 0);
        int256 fCashChangeToAccountGuess = netCashToAccount.mulInRatePrecision(rateAnchor).neg();
        for (uint8 i = 0; i < 250; i++) {
            (int256 exchangeRate, bool success) =
                _getExchangeRate(
                    totalfCash,
                    totalCashUnderlying,
                    rateScalar,
                    rateAnchor,
                    fCashChangeToAccountGuess
                );

            require(success); // dev: invalid exchange rate
            int256 delta =
                _calculateDelta(
                    netCashToAccount,
                    totalfCash,
                    totalCashUnderlying,
                    rateScalar,
                    fCashChangeToAccountGuess,
                    exchangeRate,
                    feeRate
                );

            if (delta.abs() <= maxDelta) return fCashChangeToAccountGuess;
            fCashChangeToAccountGuess = fCashChangeToAccountGuess.sub(delta);
        }

        revert("No convergence");
    }

This function break total delta into multiple small delta calculated by _getExchangeRate followed by _calculateDelta, which is applied to fCashChangeToAccountGuess each iteration for up to 250 iterations to converge. Extremely large order may require more than 250 iterations to converge thus revert with "No convergence"

Due to gas problem on above implementation, NotionalV2 use heuristic version to calculate the rate instead of the more accurate one as shown below

https://github.com/notional-finance/contracts-v2/blob/f838e3de3ca39dd55d05129f2e38ade395653890/contracts/external/actions/nTokenMintAction.sol#L266-L308

    /// @notice Lends into the market to reduce the leverage that the nToken will add liquidity at. May fail due
    /// to slippage or result in some amount of residual cash.
    function _deleverageMarket(
        CashGroupParameters memory cashGroup,
        MarketParameters memory market,
        int256 perMarketDeposit,
        uint256 blockTime,
        uint256 marketIndex
    ) private returns (int256, int256) {
        uint256 timeToMaturity = market.maturity.sub(blockTime);

        // Shift the last implied rate by some buffer and calculate the exchange rate to fCash. Hope that this
        // is sufficient to cover all potential slippage. We don't use the `getfCashGivenCashAmount` method here
        // because it is very gas inefficient.
        int256 assumedExchangeRate;
        if (market.lastImpliedRate < Constants.DELEVERAGE_BUFFER) {
            // Floor the exchange rate at zero interest rate
            assumedExchangeRate = Constants.RATE_PRECISION;
        } else {
            assumedExchangeRate = Market.getExchangeRateFromImpliedRate(
                market.lastImpliedRate.sub(Constants.DELEVERAGE_BUFFER),
                timeToMaturity
            );
        }

        int256 fCashAmount;
        {
            int256 perMarketDepositUnderlying =
                cashGroup.assetRate.convertToUnderlying(perMarketDeposit);
            // NOTE: cash * exchangeRate = fCash
            fCashAmount = perMarketDepositUnderlying.mulInRatePrecision(assumedExchangeRate);
        }
        int256 netAssetCash = market.executeTrade(cashGroup, fCashAmount, timeToMaturity, marketIndex);

        // This means that the trade failed
        if (netAssetCash == 0) {
            return (perMarketDeposit, 0);
        } else {
            // Ensure that net the per market deposit figure does not drop below zero, this should not be possible
            // given how we've calculated the exchange rate but extra caution here
            int256 residual = perMarketDeposit.add(netAssetCash);
            require(residual >= 0); // dev: insufficient cash
            return (residual, fCashAmount);
        }
    }

This function use a heuristic way with some post check condition to calculate exchange rate to fCash. Heuristic is known to be inaccurate in some case. You also mentioned that "Hope that this is sufficient to cover all potential slippage." which mean a formal proof is not given, you just hope that edge cases don't exists.

Without a formal proof, edge cases of high slippage that broke the logic which is caused by flash loan can be happened.

Tools Used

VS Code

Recommended Mitigation Steps

You should started an audit contest for NotionalV2 in code4rena. Wardens will help you review the mathematic equation. But it is very complex, some corner cases may be missing.

code-423n4 / 2022-06-notional-coop-findings

Single point of failure in NotionalV2 as it aren't audited with code4rena yet. It may expose some vulnerability to Flash loan attack. #126