Notional x Index Coop - Chom's results

Infinite approval to NotionalV2 may cause unnecessary risk

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashBase.sol#L66-L74

        // Set approvals for Notional. It is possible for an asset token address to equal the underlying
        // token address when there is no money market involved.
        assetToken.safeApprove(address(NotionalV2), type(uint256).max);
        if (
            address(assetToken) != address(underlyingToken) &&
            address(underlyingToken) != Constants.ETH_ADDRESS
        ) {
            underlyingToken.safeApprove(address(NotionalV2), type(uint256).max);
        }

This give infinite approval on assetToken and underlyingToken to be transferred by NotionalV2. This create unnecessary risk if NotionalV2 got hacked. It may steal all tokens in the wrapped fCash contract.

Consider approve on each function instead of global infinite approve will be safer.

Missing check for market index = 0, isIdiosyncratic = cannot trade

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#L274-L296

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashBase.sol#L107-L119

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

    /// @dev Sells an fCash share back on the Notional AMM
    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);
    }

    /// @notice Returns the current market index for this fCash asset. If this returns
    /// zero that means it is idiosyncratic and cannot be traded.
    function getMarketIndex() public view override returns (uint8) {
        (uint256 marketIndex, bool isIdiosyncratic) = DateTime.getMarketIndex(
            Constants.MAX_TRADED_MARKET_INDEX,
            getMaturity(),
            block.timestamp
        );

        if (isIdiosyncratic) return 0;
        // Market index as defined does not overflow this conversion
        return uint8(marketIndex);
    }

If market index = 0 or isIdiosyncratic = cannot trade. But it is missing checks in _mintInternal and _sellfCash.

Consider adding require(getMarketIndex() > 0, ...) to _mintInternal and _sellfCash.

Unauthorized user may deploy wrappedFCash

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol#L26-L37

    function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
        address _computedWrapper = computeAddress(currencyId, maturity);

        if (Address.isContract(_computedWrapper)) {
            // If wrapper has already been deployed then just return it's address
            return _computedWrapper;
        } else {
            address wrapper = Create2.deploy(0, SALT, _getByteCode(currencyId, maturity));
            emit WrapperDeployed(currencyId, maturity, wrapper);
            return wrapper;
        }
    }

deployWrapper is open to all, wrappedfCash that doesn't intended to be open for trade may be deployed by someone else. Consider adding some authorization.

Using outdated openzeppelin version

"@openzeppelin/contracts": "^3.4.2-solc-0.7",

Using outdated openzeppelin may be prone to some vulnerable as seen here https://security.snyk.io/vuln/SNYK-JS-OPENZEPPELINCONTRACTSUPGRADEABLE-2320177

And using "@openzeppelin/contracts" may cause your contract into some upgradability issues.

Consider upgrading @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to version 4.4.1 or higher.

Using Clone instead of CREATE2

CREATE2 is consuming a lot of gas while Clone consume 10 times less gas than CREATE2

The WrappedFCash contract already used initialize pattern, so it is already compatible with Clone.

Clone is smaller than Beacon.

Read more: https://blog.logrocket.com/cloning-solidity-smart-contracts-factory-pattern/

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol#L26-L37

    function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
        address _computedWrapper = computeAddress(currencyId, maturity);

        if (Address.isContract(_computedWrapper)) {
            // If wrapper has already been deployed then just return it's address
            return _computedWrapper;
        } else {
            address wrapper = Create2.deploy(0, SALT, _getByteCode(currencyId, maturity));
            emit WrapperDeployed(currencyId, maturity, wrapper);
            return wrapper;
        }
    }

This part use Create2 to deploy Beacon, can be optimized to use Clone.

You may use clone with any kind of upgradable proxy to achieve upgradability.

Use solidity custom error instead of revert message

It not only provide better gas optimization but also reduce contract size

https://blog.soliditylang.org/2021/04/21/custom-errors/

Applied to all area with require(...)

require(cashGroup.maxMarketIndex > 0, "Invalid currency");

        require(
            DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp),
            "Invalid maturity"
        );

require(pvExternal >= 0);

require(!hasMatured(), "fCash matured");

... any many more ...

Can be written like this

// Before contract definition
error Matured();

// Inside a function
if (hasMatured()) {
    revert Matured();
}

Duplicated function call on convertToShares

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

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

Noticed that totalSupply() is called twice. Can be optimized to

    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 * supply) / totalAssets();
    }