QA Report

[code423n4]

Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol:
  526:             uint88(_fCashAmount),

notional-wrapped-fcash/contracts/wfCashBase.sol:
  118:         return uint8(marketIndex);

Notice that a solution has been coded here:

File: wfCashLogic.sol
315:     function _safeUint88(uint256 x) internal pure returns (uint88) {
316:         require(x <= uint256(type(uint88).max));
317:         return uint88(x);
318:     }

A similar solution would be enough.

Missing address(0) checks

In the constructors, the solution never checks for address(0) when setting the value of immutable variables. I suggest adding those checks.

List of immutable variables:

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol:
  121:     IWrappedfCashFactory public immutable wrappedfCashFactory;
  122:     IERC20 public immutable weth;

notional-wrapped-fcash/contracts/wfCashBase.sol:
  20:     INotionalV2 public immutable NotionalV2;
  21:     WETH9 public immutable WETH;

notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol:
  11:     address public immutable BEACON;

Unbounded loop on array can lead to DoS

As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

index-coop-notional-trade-module/contracts/protocol/SetToken.sol:
  176:         for (uint256 i = 0; i < _modules.length; i++) {
  181:         for (uint256 j = 0; j < _components.length; j++) {
  220:         components.push(_component);
  252:         componentPositions[_component].externalPositionModules.push(_positionModule);
  411:         modules.push(msg.sender);
  485:         for (uint256 i = 0; i < components.length; i++) {
  502:             for (uint256 j = 0; j < externalModules.length; j++) {
  527:         for (uint256 i = 0; i < externalModules.length; i++) {
  604:         for (uint256 i = 0; i < components.length; i++) {
  614:             for (uint256 j = 0; j < externalModules.length; j++) {
  636:         for (uint256 i = 0; i < components.length; i++) {

index-coop-notional-trade-module/contracts/protocol/modules/v1/DebtIssuanceModule.sol:
  278:         issuanceSettings[_setToken].moduleIssuanceHooks.push(msg.sender);
  656:         for (uint256 i = 0; i < issuanceHooks.length; i++) {
  666:         for (uint256 i = 0; i < issuanceHooks.length; i++) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

Lack of event emission after critical initialize() functions

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions:

index-coop-notional-trade-module/contracts/protocol/modules/v1/DebtIssuanceModule.sol:
  305:     function initialize(

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol:
  219:     function initialize(

notional-wrapped-fcash/contracts/wfCashBase.sol:
  35:     function initialize(uint16 currencyId, uint40 maturity) external override initializer {

Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

index-coop-notional-trade-module/contracts/protocol/modules/v1/DebtIssuanceModule.sol:
  234:     function updateIssueFee(
  255:     function updateRedeemFee(

Use a more recent version of solidity to get string.concat() (0.8.12 vs 0.8.11)

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

notional-wrapped-fcash/contracts/wfCashBase.sol:
   2: pragma solidity 0.8.11;
  59:             string(abi.encodePacked("Wrapped f", _symbol, " @ ", _maturity)),
  61:             string(abi.encodePacked("wf", _symbol, ":", _maturity)),

notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol:
   2: pragma solidity 0.8.11;
  23:         return abi.encodePacked(type(nBeaconProxy).creationCode, abi.encode(BEACON, initCallData));

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons. Details below

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

index-coop-notional-trade-module/contracts/protocol/SetToken.sol:
  205:         returns (bytes memory _returnValue) //@audit NC: unused named return
  211:         return _returnValue; //@audit NC: unused named return

notional-wrapped-fcash/contracts/wfCashBase.sol:
  124:     function getUnderlyingToken() public view override returns (IERC20 underlyingToken, int256 underlyingPrecision) { //@audit NC: unused named return
  129:             return (IERC20(asset.tokenAddress), asset.decimals); //@audit NC: unused named return
  131:             return (IERC20(underlying.tokenAddress), underlying.decimals); //@audit NC: unused named return

  137:     function getAssetToken() public view override returns (IERC20 assetToken, int256 underlyingPrecision, TokenType tokenType) { //@audit NC: unused named return
  139:         return (IERC20(asset.tokenAddress), asset.decimals, asset.tokenType); //@audit NC: unused named return

notional-wrapped-fcash/contracts/wfCashERC4626.sol:
  52:     function convertToShares(uint256 assets) public view override returns (uint256 shares) { //@audit NC: unused named return
  57:             return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue; //@audit NC: unused named return
  60:         return (assets * totalSupply()) / totalAssets(); //@audit NC: unused named return

  64:     function convertToAssets(uint256 shares) public view override returns (uint256 assets) { //@audit NC: unused named return
  68:             return _getPresentValue(shares); //@audit NC: unused named return
  71:         return (shares * totalAssets()) / supply; //@audit NC: unused named return

The pragmas used are not the same everywhere

index-coop-notional-trade-module/contracts/protocol/SetToken.sol:
  19: pragma solidity 0.6.10;

index-coop-notional-trade-module/contracts/protocol/integration/wrap/notional/WrappedfCash.sol:
  2: pragma solidity 0.8.11;

index-coop-notional-trade-module/contracts/protocol/integration/wrap/notional/WrappedfCashFactory.sol:
  2: pragma solidity 0.8.11;

index-coop-notional-trade-module/contracts/protocol/modules/v1/DebtIssuanceModule.sol:
  19: pragma solidity 0.6.10;

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol:
  19: pragma solidity 0.6.10;

notional-wrapped-fcash/contracts/wfCashBase.sol:
  2: pragma solidity 0.8.11;

notional-wrapped-fcash/contracts/wfCashERC4626.sol:
  2: pragma solidity ^0.8.0;

notional-wrapped-fcash/contracts/wfCashLogic.sol:
  2: pragma solidity 0.8.11;

notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol:
  2: pragma solidity 0.8.11;

notional-wrapped-fcash/interfaces/notional/INotionalV2.sol:
  2: pragma solidity ^0.8.11;

notional-wrapped-fcash/interfaces/notional/IWrappedfCash.sol:
  2: pragma solidity ^0.8.0;

Avoid floating pragmas: the version should be locked

notional-wrapped-fcash/contracts/wfCashERC4626.sol:
  2: pragma solidity ^0.8.0;

notional-wrapped-fcash/interfaces/notional/INotionalV2.sol:
  2: pragma solidity ^0.8.11;

notional-wrapped-fcash/interfaces/notional/IWrappedfCash.sol:
  2: pragma solidity ^0.8.0;

[ckoopmann]

Unbounded loop on array can lead to DoS I aknowledged else where.

Most of the other set protocol related issues are out of scope and the issue The pragmas used are not the same everywhere is not applicable since the fCashWrapper and the NotionalTradeModule are separately deployed modules from totally separate codebases.