Notional x Index Coop - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L104-L152

Vulnerability details

Impact

attacker can make function onERC1155Received() to revert all the time by transferring other fcash directly in NotionalV2 to wfCash's address before its deployment (address is predictable and also attacker can perform front-running) and make contract have multiple assets in its NotionalV2.getAccountPortfolio() and the conditions in onERC1155Received() will not met and code will revert.

There may exist other DOS attacks with this attack vector (sending custom fcash to contract address before its deployment) based on NotionalV2 behavior on withdraw, settleAccount, transfer,...

Proof of Concept

This is onERC1155Received() code in wfCashLogic():

    function onERC1155Received(
        address _operator,
        address _from,
        uint256 _id,
        uint256 _value,
        bytes calldata _data
    ) external nonReentrant returns (bytes4) {
        uint256 fCashID = getfCashId();
        // Only accept erc1155 transfers from NotionalV2
        require(
            msg.sender == address(NotionalV2) &&
            // Only accept the fcash id that corresponds to the listed currency and maturity
            _id == fCashID &&
            // Protect against signed value underflows
            int256(_value) > 0,
            "Invalid"
        );

        // Double check the account's position, these are not strictly necessary and add gas costs
        // but might be good safe guards
        AccountContext memory ac = NotionalV2.getAccountContext(address(this));
        PortfolioAsset[] memory assets = NotionalV2.getAccountPortfolio(address(this));
        require(
            ac.hasDebt == 0x00 &&
            assets.length == 1 &&
            EncodeDecode.encodeERC1155Id(
                assets[0].currencyId,
                assets[0].maturity,
                assets[0].assetType
            ) == fCashID
        );

        // Update per account fCash balance, calldata from the ERC1155 call is
        // passed via the ERC777 interface.
        bytes memory userData;
        bytes memory operatorData;
        if (_operator == _from) userData = _data;
        else operatorData = _data;

        // We don't require a recipient ack here to maintain compatibility
        // with contracts that don't support ERC777
        _mint(_from, _value, userData, operatorData, false);

        // This will allow the fCash to be accepted
        return ERC1155_ACCEPTED;
    }

As you can see code calls assets = NotionalV2.getAccountPortfolio(address(this)); to get contract portfolio in NotionalV2 and then checks that assets.length == 1 is True. so if contract has multiple assets (fcash) in his portfolio then this code will revert. onERC1155Received() has some checks to ensure that no one can transfers other fcash tokens to contract address but attacker can send those tokens before contract deployment. because contract address is predictable based on Create2 logic. This is computeAddress() code in WrappedfCashFactory:

    function computeAddress(uint16 currencyId, uint40 maturity) public view returns (address) {
        return Create2.computeAddress(SALT, keccak256(_getByteCode(currencyId, maturity)));
    }

As you can see contract address is determined by SALT, currencyId, maturity. so attacker can easily send some other fcash tokens to possible address before their deployments and perform DOS. attacker can perform front-running for deployment transaction and perform this DOS on all the deployed contracts.

Tools Used

VIM

Recommended Mitigation Steps

remove this check from onERC1155Received() or add some mechanism to withdraw other fcash tokens in initialization.

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L177-L184 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L168-L175 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L225-L241

Vulnerability details

Impact

For some fcash the asset token is underlying token (asset.tokenType == TokenType.NonMintable) and NotionalV2 will not handle minting with useUnderlying==True for those fcashs (according to what I asked from sponsor). In summery most of the logics in wfCashERC4626 will not work for those fcash tokens.

when for some fcash asset token is underlying token, all calls to NotionalV2 should be with useUnderlying==False. but deposit() and mint() in wfCashERC4626 contract call _mintInternal() with useUnderlying==True and it calls NotionalV2.batchLend() with depositUnderlying==true so the NotionV2 call will fail for fcash tokens that asset token is underlying token and it would cause that deposit() and mint() logic wfCashERC4626 will not work and contract will be useless for those tokens. _redeemInternal() issue is similar and it calls _burn() with redeemToUnderlying: true which execution eventually calls NotionalV2.batchBalanceAndTradeAction() with toUnderlying=True which will revert so _redeemInternal() will fail and because withdraw() and redeem use it, so they will not work too for those fcash tokens that asset token is underlying token.

Proof of Concept

This is deposit() and mint() code in wfCashERC4626:

    /** @dev See {IERC4626-deposit} */
    function deposit(uint256 assets, address receiver) public override returns (uint256) {
        uint256 shares = previewDeposit(assets);
        // Will revert if matured
        _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
        emit Deposit(msg.sender, receiver, assets, shares);
        return shares;
    }

    /** @dev See {IERC4626-mint} */
    function mint(uint256 shares, address receiver) public override returns (uint256) {
        uint256 assets = previewMint(shares);
        // Will revert if matured
        _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
        emit Deposit(msg.sender, receiver, assets, shares);
        return assets;
    }

As you can see they both call _mintInternal() with last parameter as true which is useUnderlying's value. This is _mintInternal() code:

    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);
    }

As you can see it calls NotionalV2 functions with useUnderlying=True but according to sponsor clarification NotionalV2 would fail and revert for those calls because useUnderlying=True and fcash's asset token is underlying token (asset.tokenType == TokenType.NonMintable). So in summery for fcash tokens which asset token is underlying token NotionalV2 won't handle calls which include useUnderlying==True but in wfCashERC4626 contract functions like deposit(), mint(), withdraw() and redeem() they all uses useUnderlying==True always so wfCashERC4626 won't work for those specific type of tokens which asset token is underlying token(asset.tokenType == TokenType.NonMintable)

the detail explanations for functions withdraw() and redeem() are similar.

Tools Used

VIM

Recommended Mitigation Steps

check that if for that fcash token asset token is underlying token or not and set useUnderlying based on that.

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L558-L575

Vulnerability details

Impact

For some fcash the asset token is underlying token (asset.tokenType == TokenType.NonMintable) and NotionalV2 will not handle minting or burning when it is called with useUnderlying==True for those fcashs (according to what I asked from sponsor). In summery most of the logics in NotionalTradeModule will not work for those fcash tokens because _isUnderlying() returns true result for those tokens which would make NotionalTradeModule's logic for mintFCashPosition() and redeemFCashPosition() will eventually call redeemToUnderlying() and mintViaUnderlying() in wfCashLogic and those function in wfCashLogic will call NotionalV2 with useUnderlying==True and NotionalV2 will fail and revert for fcash tokens which asset token is underlying token, so the whole transaction will fail and _mintFCashPosition() and _redeemFCashPosition() logic in NotionalTradeModule will not work for those fcash tokens and manager can't add them to set protocol.

Proof of Concept

when for some fcash asset token is underlying token, all calls to NotionalV2 should be with useUnderlying==False. but _isUnderlying() in NotionalTradeModule contract first check that isUnderlying = _paymentToken == underlyingToken so for fcash tokens where asset token is underlying token it is going to return isUnderlying==True. let's assume that for some specific fcash asset token is underlying token (asset.tokenType == TokenType.NonMintable) and follow the code execution. This is _isUnderlying() code in NotionalTradeModule:

    function _isUnderlying(
        IWrappedfCashComplete _fCashPosition,
        IERC20 _paymentToken
    )
    internal
    view
    returns(bool isUnderlying)
    {
        (IERC20 underlyingToken, IERC20 assetToken) = _getUnderlyingAndAssetTokens(_fCashPosition);
        isUnderlying = _paymentToken == underlyingToken;
        if(!isUnderlying) {
            require(_paymentToken == assetToken, "Token is neither asset nor underlying token");
        }
    }

As you can see it calls _getUnderlyingAndAssetTokens() and then check _paymentToken == underlyingToken to see that if payment token is equal to underlyingToken. _getUnderlyingAndAssetTokens() uses getUnderlyingToken() and getAssetToken() in wfCashBase. This is getUnderlyingToken() code in wfCashBase:

    /// @notice Returns the token and precision of the token that this token settles
    /// to. For example, fUSDC will return the USDC token address and 1e6. The zero
    /// address will represent ETH.
    function getUnderlyingToken() public view override returns (IERC20 underlyingToken, int256 underlyingPrecision) {
        (Token memory asset, Token memory underlying) = NotionalV2.getCurrency(getCurrencyId());

        if (asset.tokenType == TokenType.NonMintable) {
            // In this case the asset token is the underlying
            return (IERC20(asset.tokenAddress), asset.decimals);
        } else {
            return (IERC20(underlying.tokenAddress), underlying.decimals);
        }
    }

As you can see for our specific fcash token this function will return asset token as underlying token. so for this specific fcash token, the asset token and underlying token will be same in _isUnderlying() of NationalTradeModule but because code first check isUnderlying = _paymentToken == underlyingToken so the function will return isUnderlying=True as a result for our specific fcash token (which asset token is underlying token) This is _mintFCashPosition() and _redeemFCashPosition() code in NotionalTradeModule :

    /**
     * @dev Redeem a given fCash position from the specified send token (either underlying or asset token)
     * @dev Alo adjust the components / position of the set token accordingly
     */
    function _mintFCashPosition(
        ISetToken _setToken,
        IWrappedfCashComplete _fCashPosition,
        IERC20 _sendToken,
        uint256 _fCashAmount,
        uint256 _maxSendAmount
    )
    internal
    returns(uint256 sentAmount)
    {
        if(_fCashAmount == 0) return 0;

        bool fromUnderlying = _isUnderlying(_fCashPosition, _sendToken);


        _approve(_setToken, _fCashPosition, _sendToken, _maxSendAmount);

        uint256 preTradeSendTokenBalance = _sendToken.balanceOf(address(_setToken));
        uint256 preTradeReceiveTokenBalance = _fCashPosition.balanceOf(address(_setToken));

        _mint(_setToken, _fCashPosition, _maxSendAmount, _fCashAmount, fromUnderlying);


        (sentAmount,) = _updateSetTokenPositions(
            _setToken,
            address(_sendToken),
            preTradeSendTokenBalance,
            address(_fCashPosition),
            preTradeReceiveTokenBalance
        );

        require(sentAmount <= _maxSendAmount, "Overspent");
        emit FCashMinted(_setToken, _fCashPosition, _sendToken, _fCashAmount, sentAmount);
    }

    /**
     * @dev Redeem a given fCash position for the specified receive token (either underlying or asset token)
     * @dev Alo adjust the components / position of the set token accordingly
     */
    function _redeemFCashPosition(
        ISetToken _setToken,
        IWrappedfCashComplete _fCashPosition,
        IERC20 _receiveToken,
        uint256 _fCashAmount,
        uint256 _minReceiveAmount
    )
    internal
    returns(uint256 receivedAmount)
    {
        if(_fCashAmount == 0) return 0;

        bool toUnderlying = _isUnderlying(_fCashPosition, _receiveToken);
        uint256 preTradeReceiveTokenBalance = _receiveToken.balanceOf(address(_setToken));
        uint256 preTradeSendTokenBalance = _fCashPosition.balanceOf(address(_setToken));

        _redeem(_setToken, _fCashPosition, _fCashAmount, toUnderlying);


        (, receivedAmount) = _updateSetTokenPositions(
            _setToken,
            address(_fCashPosition),
            preTradeSendTokenBalance,
            address(_receiveToken),
            preTradeReceiveTokenBalance
        );


        require(receivedAmount >= _minReceiveAmount, "Not enough received amount");
        emit FCashRedeemed(_setToken, _fCashPosition, _receiveToken, _fCashAmount, receivedAmount);

    }

As you can see they both uses _isUnderlying() to find out that if _sendToken is asset token or underlying token. for our specific fcash token, the result of _isUnderlying() will be True and _mintFCashPosition() and _redeemFCashPosition() will call _mint() and _redeem() with toUnderlying set as True. This is _mint() and _redeem() code:

    /**
     * @dev Invokes the wrappedFCash token's mint function from the setToken
     */
    function _mint(
        ISetToken _setToken,
        IWrappedfCashComplete _fCashPosition,
        uint256 _maxAssetAmount,
        uint256 _fCashAmount,
        bool _fromUnderlying
    )
    internal
    {
        uint32 minImpliedRate = 0;

        bytes4 functionSelector = 
            _fromUnderlying ? _fCashPosition.mintViaUnderlying.selector : _fCashPosition.mintViaAsset.selector;
        bytes memory mintCallData = abi.encodeWithSelector(
            functionSelector,
            _maxAssetAmount,
            uint88(_fCashAmount),
            address(_setToken),
            minImpliedRate,
            _fromUnderlying
        );
        _setToken.invoke(address(_fCashPosition), 0, mintCallData);
    }

    /**
     * @dev Redeems the given amount of fCash token on behalf of the setToken
     */
    function _redeem(
        ISetToken _setToken,
        IWrappedfCashComplete _fCashPosition,
        uint256 _fCashAmount,
        bool _toUnderlying
    )
    internal
    {
        uint32 maxImpliedRate = type(uint32).max;

        bytes4 functionSelector =
            _toUnderlying ? _fCashPosition.redeemToUnderlying.selector : _fCashPosition.redeemToAsset.selector;
        bytes memory redeemCallData = abi.encodeWithSelector(
            functionSelector,
            _fCashAmount,
            address(_setToken),
            maxImpliedRate
        );
        _setToken.invoke(address(_fCashPosition), 0, redeemCallData);
    }

As you can see they are using _toUnderlying value to decide calling between (mintViaUnderlying() or mintViaAsset()) and (redeemToUnderlying() or redeemToAsset()), for our specific fcash _toUnderlying will be True so those functions will call mintViaUnderlying() and redeemToUnderlying() in wfCashLogic. mintViaUnderlying() and redeemToUnderlying() in wfCashLogic execution flow eventually would call NotionalV2 functions with useUnderlying=True for this specific fcash token, but NotionalV2 will revert for that call because for that fcash token asset token is underlying token and NotionalV2 can't handle calls with useUnderlying==True for that fcash Token. This will cause all the transaction to fail and manager can't call redeemFCashPosition() or mintFCashPosition() functions for those fcash tokens that asset token is underlying token. In summery NotionalTradeModule logic will not work for all fcash tokens becasue the logic of _isUnderlying() is wrong for fcash tokens that asset token is underlying token.

Tools Used

VIM

Recommended Mitigation Steps

Change the logic of _isUnderlying() in NotionalTradeModule so it returns correct results for all fcash tokens. one simple solution can be that it first check payment token value with asset token value.

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L51-L61 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L63-L72

Vulnerability details

Impact

Functions convertToShares() and convertToAssets() in wfCashERC4626 will return wrong results if totalSupply is equal to 0 and fcash is matured. function previewWithdraw() calls convertToShares() and returns its result when hasMatured() is True and previewRedeem() calls convertToAssets() and returns its result when hasMatured() is True. so convertToShares() and convertToAssets() should be able to calculate correct result even if fcash is matured and totalSupply() is 0. but convertToShares() and convertToAssets() calls _getPresentValue() which calls NotionalV2.getPresentfCashValue() and don't check for the case where isMatured() is True.

Proof of Concept

This is convertToShares() and convertToAssets() code in wfCashERC4626:

    /** @dev See {IERC4626-convertToShares} */
    function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        uint256 supply = totalSupply();
        if (supply == 0) {
            // Scales assets by the value of a single unit of fCash
            uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
            return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
        }

        return (assets * totalSupply()) / totalAssets();
    }

    /** @dev See {IERC4626-convertToAssets} */
    function convertToAssets(uint256 shares) public view override returns (uint256 assets) {
        uint256 supply = totalSupply();
        if (supply == 0) {
            // Catch the edge case where totalSupply causes a divide by zero error
            return _getPresentValue(shares);
        }

        return (shares * totalAssets()) / supply;
    }

As you can see when totalSupply() == 0 is True code calls _getPresentValue() to calculate the result. This is _getPresentValue() code:

    function _getPresentValue(uint256 fCashAmount) private view returns (uint256) {
        (/* */, int256 precision) = getUnderlyingToken();
        // Get the present value of the fCash held by the contract, this is returned in 8 decimal precision
        (uint16 currencyId, uint40 maturity) = getDecodedID();
        int256 pvInternal = NotionalV2.getPresentfCashValue(
            currencyId,
            maturity,
            int256(fCashAmount), // total supply cannot overflow as fCash overflows at uint88
            block.timestamp,
            false
        );

        int256 pvExternal = (pvInternal * precision) / Constants.INTERNAL_TOKEN_PRECISION;
        // PV should always be >= 0 since we are lending
        require(pvExternal >= 0);
        return uint256(pvExternal);
    }

it calls NotionalV2.getPresentfCashValue() to calculate result. but if fcash is matured there is no guarantee that NotionalV2.getPresentfCashValue() will calculate correct results. so in convertToShares() and convertToAssets() if fcash is matured and totalSupply() is 0 then code should use another logic than calling _getPresentValue(). This is previewWithdraw() and previewRedeem() code, other logics uses this two function too:

    /** @dev See {IERC4626-previewWithdraw} */
    function previewWithdraw(uint256 assets) public view override returns (uint256 shares) {
        if (hasMatured()) {
            shares = convertToShares(assets);
        } else {
            // If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
            (uint16 currencyId, uint40 maturity) = getDecodedID();
            (shares, /* */, /* */) = NotionalV2.getfCashBorrowFromPrincipal(
                currencyId,
                assets,
                maturity,
                0,
                block.timestamp,
                true
            );
        }
    }

    /** @dev See {IERC4626-previewRedeem} */
    function previewRedeem(uint256 shares) public view override returns (uint256 assets) {
        if (hasMatured()) {
            assets = convertToAssets(shares);
        } else {
            // If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
            (uint16 currencyId, uint40 maturity) = getDecodedID();
            (assets, /* */, /* */, /* */) = NotionalV2.getPrincipalFromfCashBorrow(
                currencyId,
                shares,
                maturity,
                0,
                block.timestamp
            );
        }
    }

As you can see if hasMatured() was True they call convertToShares() and convertToAssets(), So they should be able to calculate correct results in each state. right now if totalSupply() was 0 and fcash is matured, code uses _getPresentValue() which is not for matured fcash.

Tools Used

VIM

Recommended Mitigation Steps

add more logic to check for matured fcash and handle it separately.