QA Report

[code423n4]

QA (LOW & NON-CRITICAL)

[L-01] Use of floating pragma

Floating Pragma used in wfCashERC4626.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
Reference

[L-02] Different and outdated compiler versions

The scoped contracts have different solidity compiler ranges (0.6.10 - 0.8.11) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior. Current Solidity version available is 0.8.14

[L-03] Missing checks for contract existence

OpenZeppelin's safeTransfer and safeTransferFrom functions use functionCall of Address.sol which is a low level call.
However, it's stated that the target must be a contract and code existence should be checked prior using. Reference
It's advised to check code existence prior calling safeTransfer or safeTransferFrom as below;
In wfCashLogic.sol,

84: token.safeTransferFrom(msg.sender, address(this), depositAmountExternal);
311: token.safeTransfer(receiver, tokensTransferred);

[L-04] Missing of zero address checks for the immutables

No address(0) or Zero value check at the constructors of:
In WrappedfCashFactory.sol;

    constructor(address _beacon) {
        BEACON = _beacon;
    }

In wfCashBase.sol;

    constructor(INotionalV2 _notional, WETH9 _weth) initializer {
        NotionalV2 = _notional;
        WETH = _weth;
    }

In wfCashERC4626.sol;

constructor(INotionalV2 _notional, WETH9 _weth) wfCashLogic(_notional, _weth) {}

In wfCashLogic.sol;

constructor(INotionalV2 _notional, WETH9 _weth) wfCashBase(_notional, _weth) {}

In NotionalTradeModule.sol;

constructor(
        IController _controller,
        IWrappedfCashFactory _wrappedfCashFactory,
        IERC20 _weth

    )
        public
        ModuleBase(_controller)
    {
        wrappedfCashFactory = _wrappedfCashFactory;
        weth = _weth;
    }

[L-05] Missing address comparison after Create2 function

WrappedfCashFactory uses Solidity's Create2 function to deploy the wrappers. In case of a deployment failure it will revert as zero address and will throw an error. However, a good practice is to compare the _computedWrapper address with the deployed wrapper same as in Solidity's official page here

contract C {
    function createDSalted(bytes32 salt, uint arg) public {
        // This complicated expression just tells you how the address
        // can be pre-computed. It is just there for illustration.
        // You actually only need ``new D{salt: salt}(arg)``.
        address predictedAddress = address(uint160(uint(keccak256(abi.encodePacked(
            bytes1(0xff),
            address(this),
            salt,
            keccak256(abi.encodePacked(
                type(D).creationCode,
                abi.encode(arg)
            ))
        )))));

        D d = new D{salt: salt}(arg);
        require(address(d) == predictedAddress);
    }
}

[L-06] Unbounded loop on array can lead to DoS

Programming patterns such as looping over arrays of unknown size may lead to DoS when the gas cost of execution exceeds the block gas limit. Reference
Instances in NotionalTradeModule.sol;

1. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L246-L259
2. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L385-L410 (This function is inlined to externally called functions)

[L-07] External Calls inside a loop

Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded.Reference

Instances in NotionalTradeModule.sol;

1. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L219-L241
2. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L246-L259

[L-08] wfCashERC4626 is not as per EIP4626 standard

EIP4626 requires deposit and withdraw functions having event with indexed parameters for caller, owner and receiver. However, this pattern is not followed.
Reference

[N-01] Some require statements don't throw error message

Some require statements in the scoped contracts don't throw error. In case of any error pops up, it will not be possible to know the error source. The list is as below;

1. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L42
2. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L245
3. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L129
4. https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L316

[N-02] Scoped contracts are missing proper NatSpec comments

The scoped contracts are missing proper NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference

[jeffywu]

A couple invalid points on this report

L5: OZ create2 reverts on failed deployment already
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Create2.sol#L42

L3 checking for contract existence would increase gas costs. Tokens are only white listed if they actually exist.