QA Report

[code423n4]

Summary

We list 3 low-critical findings and 1 non-critical findings:

• (Low) `ReentrancyGuard` is not upgradable
• (Low) No relationship between depositAmountExternal and fCashAmount
• (Low) floating pragma
• (Non) mintCallData has wrong number of arguments in NotionalTradeModule\_mint()

(Low) ReentrancyGuard is not upgradable

The contract wfCashERC4626 uses ERC777Upgradeable but ReentrancyGuard is not upgradable. It may cause wrong slots of reserved space.

Proof of Concept

In wfCashBase it use upgradable ERC777:

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashBase.sol#L16

abstract contract wfCashBase is ERC777Upgradeable, IWrappedfCash {

But in wfCashLogic it doesn't use upgradable ReentrancyGuard: https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L11

abstract contract wfCashLogic is wfCashBase, ReentrancyGuard {

Tools Used

None

Recommended Mitigation Steps

Use ReentrancyGuardUpgradeable.sol

(Low) No relationship between depositAmountExternal and fCashAmount

Impact

depositAmountExternal and fCashAmount are both the arguments of wfCashLogic\mintViaUnderlying() and wfCashLogic\mintViaAsset() . But they doesn't make sure that depositAmountExternal is enough for fCashAmount . We cannot guarantee that NotionalV2 will check that.

Proof of Concept

It directly mint fCashAmount tokens

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L50-L102

    function _mintInternal(
        uint256 depositAmountExternal,
        uint88 fCashAmount,
        address receiver,
        uint32 minImpliedRate,
        bool useUnderlying
    ) internal nonReentrant {
        ...
        _mint(receiver, fCashAmount, "", "", false);
        ...
    }

Tools Used

None

Recommended Mitigation Steps

Calculate the right amount of token in wfCashLogic.sol

(Low) floating pragma

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L2

pragma solidity ^0.8.0;

Recommended Mitigation Steps

Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.8.0;

(Non) mintCallData has wrong number of arguments in NotionalTradeModule\_mint()

Impact

mintCallData has five arguments for _fCashPosition.mintViaUnderlying.selector and _fCashPosition.mintViaAsset.selector . But those functions actually take four arguments.

Proof of Concept

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L523-L530

        bytes memory mintCallData = abi.encodeWithSelector(
            functionSelector,
            _maxAssetAmount,
            uint88(_fCashAmount),
            address(_setToken),
            minImpliedRate,
            _fromUnderlying
        );

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L27-L32

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L41-L46

    function mintViaAsset(
        uint256 depositAmountExternal,
        uint88 fCashAmount,
        address receiver,
        uint32 minImpliedRate
    ) external override {
        _mintInternal(depositAmountExternal, fCashAmount, receiver, minImpliedRate, false);
    }

    function mintViaUnderlying(
        uint256 depositAmountExternal,
        uint88 fCashAmount,
        address receiver,
        uint32 minImpliedRate
    ) external override {
        _mintInternal(depositAmountExternal, fCashAmount, receiver, minImpliedRate, true);
    }

Tools Used

None

Recommended Mitigation Steps

Fix the number of arguments in NotionalTradeModule\_mint()

        bytes memory mintCallData = abi.encodeWithSelector(
            functionSelector,
            _maxAssetAmount,
            uint88(_fCashAmount),
            address(_setToken),
            minImpliedRate
        );

[jeffywu]

Good catch on the reentrancy guard

[ckoopmann]

Flagging that this report contains mintCallData has wrong number of arguments in NotionalTradeModule\_mint() which I confirmed as an issue elsewhere. (Although I think this is in fact more of a QA topic rather than the Mid-risk issue it was flagged as elsewhere).