Notional x Index Coop - Cityscape's results

Non-Critical

Clarity

/// @dev Storage slot for fCash id. Read only and set on initialization @audit Read-only

proposed change:

/// @dev Storage slot for fCash id. Read-only and set on initialization

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

Typos

/// @notice Returns the components of the fCash idd @audit idd?

proposed change:

/// @notice Returns the components of the fCash ID

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

// but might be good safe guards @audit safeguards

proposed change:

// but might be good safeguards 

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

* @dev MANGER ONLY: Initialize given SetToken with initial list of registered fCash positions @audit MANAGER ONLY?

proposed change:

* @dev MANAGER ONLY: Initialize given SetToken with initial list of registered fCash positions @audit MANAGER ONLY?

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

// Mapping for a set token, wether or not to redeem to underlying upon reaching maturity @audit whether

proposed change:

// Mapping for a set token, whether or not to redeem to underlying upon reaching maturity

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

Grammatical Errors

// is more gas efficient (does not require and additional redeem call to asset tokens). If using cETH @audit and additional? doesn't seem right, maybe an additional

proposed change:

// is more gas efficient (does not require an additional redeem call to asset tokens). If using cETH 

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

* @dev Checks if a given address is an fCash position that was deployed from the factory @audit a fCash

proposed change:

* @dev Checks if a given address is a fCash position that was deployed from the factory @audit a fCash

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

Natspec Errors

The constructor lacks a param natspec for _weth

/**
     * @dev Instantiate addresses
     * @param _controller                       Address of controller contract
     * @param _wrappedfCashFactory              Address of fCash wrapper factory used to check and deploy wrappers
     */
    constructor(
        IController _controller,
        IWrappedfCashFactory _wrappedfCashFactory,
        IERC20 _weth

    )

proposed change:

/**
     * @dev Instantiate addresses
     * @param _controller                       Address of controller contract
     * @param _wrappedfCashFactory              Address of fCash wrapper factory used to check and deploy wrappers
       @param _weth                             Address of WETH contract
     */
    constructor(
        IController _controller,
        IWrappedfCashFactory _wrappedfCashFactory,
        IERC20 _weth

    )

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

Consistency

It is recommended to change the function name from getDecodedID to getDecodedId to maintain a level of naming consistency between all the Id functions

function getDecodedID() public view override returns (uint16 currencyId, uint40 maturity) {

proposed change:

function getDecodedId() public view override returns (uint16 currencyId, uint40 maturity) {

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

Low-Risk

safeApprove is deprecated in favor of safeIncreaseAllowance

According to OZ documentation safeApproved is deprecated in favor of safeIncreaseAllowance: openzeppelin-contracts/SafeERC20.sol at bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05 · OpenZeppelin/openzeppelin-contracts · GitHub

assetToken.safeApprove(address(NotionalV2), type(uint256).max);
        if (
            address(assetToken) != address(underlyingToken) &&
            address(underlyingToken) != Constants.ETH_ADDRESS
        ) {
            underlyingToken.safeApprove(address(NotionalV2), type(uint256).max);
        }

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

Gas Optimizations

Loop Optimizations

for(uint256 i = 0; i < modules.length; i++) {
            try IDebtIssuanceModule(modules[i]).registerToIssuanceModule(_setToken) {} catch {}
        }

proposed change:

for(uint256 i; i < modules.length;) {
            try IDebtIssuanceModule(modules[i]).registerToIssuanceModule(_setToken) {} catch {}
            unchecked {++i;}
        }

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

for(uint256 i = 0; i < modules.length; i++) {
            if(modules[i].isContract()){
                try IDebtIssuanceModule(modules[i]).unregisterFromIssuanceModule(setToken) {} catch {}
            }
        }

proposed change:

for(uint256 i; i < modules.length;) {
            if(modules[i].isContract()){
                try IDebtIssuanceModule(modules[i]).unregisterFromIssuanceModule(setToken) {} catch {}
            }
            unchecked {++i;}
        }

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

for(uint256 i = 0; i < positionsLength; i++) {
            if(positions[i].unit > 0) {
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    fCashPositions[j] = component;
                    j++;
                }
            }
        }

proposed change:

for(uint256 i; i < positionsLength;) {
            if(positions[i].unit > 0) {
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    fCashPositions[j] = component;
                    j++;
                }
            }
            unchecked {++i;}
        }

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

for(uint256 i = 0; i < positionsLength; i++) {
            // Check that the given position is an equity position
            if(positions[i].unit > 0) {
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    numFCashPositions++;
                }
            }
        }

proposed change:

for(uint256 i; i < positionsLength;) {
            // Check that the given position is an equity position
            if(positions[i].unit > 0) {
                address component = positions[i].component;
                if(_isWrappedFCash(component)) {
                    numFCashPositions++;
                }
            }
            unchecked {++i;}

        }

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

Default Initialized variable

uint32 minImpliedRate = 0;

proposed change:

uint32 minImpliedRate;

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

Cache Array length in memory

address[] memory modules = setToken.getModules();
        for(uint256 i = 0; i < modules.length; i++)

proposed change:

address[] memory modules = setToken.getModules();
uint modulesLength = modules.length;
        for(uint256 i = 0; i < modulesLength; i++)

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

address[] memory modules = _setToken.getModules();
        for(uint256 i = 0; i < modules.length; i++) {

proposed change:

address[] memory modules = _setToken.getModules();
uint modulesLength = modules.length;
        for(uint256 i = 0; i < modulesLength; i++) {

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