QA Report - Githubissues

Summary

Low Risk Issues

	Issue	Instances
1	Leap years may cause problems requiring upgrades	1
2	`initialize()` function does not use the `initializer` modifier	1
3	`safeApprove()` is deprecated	2
4	Missing checks for `address(0x0)` when assigning values to `address` state variables	1

Total: 5 instances over 4 issues

Non-critical Issues

	Issue	Instances
1	Missing `initializer` modifier on constructor	2
2	Missing `initializer` modifier	2
3	Contract implements interface without extending the interface	1
4	`override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings	6
5	`require()`/`revert()` statements should have descriptive reason strings	4
6	`public` functions not called by the contract should be declared `external` instead	1
7	`constant`s should be defined rather than using magic numbers	1
8	Redundant cast	1
9	Use a more recent version of solidity	2
10	Use a more recent version of solidity	2
11	Non-library/interface files should use fixed compiler versions, not floating ones	1
12	Typos	4
13	File is missing NatSpec	1
14	NatSpec is incomplete	4
15	Event is missing `indexed` fields	1
16	Not using the named return variables anywhere in the function is confusing	6

Total: 39 instances over 16 issues

Low Risk Issues

1. Leap years may cause problems requiring upgrades

I searched for a long time for any reference to leap years in the Notional documentation, Discord, and github, but couldn't find any references to it. 2023 is going to be a leap year which means the number of days in a year will be off, and therefore all maturities will no longer fall on the same day of the month. I'm assuming that the answer will be that Notional just ignores this issue and lets things slowly drift over time, but if they decide to instead shift to the closest Idiosyncratic fCash, the maturities that this code uses will no longer be valid (will be idosyncratic) and will therefore not have any liquidity pool to convert from, pre-maturity.

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/wfCashBase.sol   #1

35       function initialize(uint16 currencyId, uint40 maturity) external override initializer {
36           CashGroupSettings memory cashGroup = NotionalV2.getCashGroup(currencyId);
37           require(cashGroup.maxMarketIndex > 0, "Invalid currency");
38           // Ensure that the maturity is not past the max market index, also ensure that the maturity
39           // is not in the past. This statement will allow idiosyncratic (non-tradable) fCash assets.
40           require(
41               DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp),
42               "Invalid maturity"
43:          );

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

2. `initialize()` function does not use the `initializer` modifier

Without the modifier, the function may be called multiple times, overwriting prior initializations

There is 1 instance of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

219       function initialize(
220           ISetToken _setToken
221       )
222           external
223           onlySetManager(_setToken, msg.sender)
224:          onlyValidAndPendingSet(_setToken)

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

3. `safeApprove()` is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There are 2 instances of this issue:

File: notional-wrapped-fcash/contracts/wfCashBase.sol   #1

68:           assetToken.safeApprove(address(NotionalV2), type(uint256).max);

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

File: notional-wrapped-fcash/contracts/wfCashBase.sol   #2

73:               underlyingToken.safeApprove(address(NotionalV2), type(uint256).max);

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

4. Missing checks for `address(0x0)` when assigning values to `address` state variables

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol   #1

18:           BEACON = _beacon;

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

Non-critical Issues

1. Missing `initializer` modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors

There are 2 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

131       constructor(
132           IController _controller,
133           IWrappedfCashFactory _wrappedfCashFactory,
134           IERC20 _weth
135   
136       )
137           public
138:          ModuleBase(_controller)

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #2

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

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

2. Missing `initializer` modifier

The contract extends ReentrancyGuard/ReentrancyGuardUpgradeable but does not use the initializer modifier anywhere

There are 2 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

44:   contract NotionalTradeModule is ModuleBase, ReentrancyGuard, Ownable, IModuleIssuanceHook {

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #2

11:   abstract contract wfCashLogic is wfCashBase, ReentrancyGuard {

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

3. Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #1

/// @audit onERC1155Received()
11:   abstract contract wfCashLogic is wfCashBase, ReentrancyGuard {

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

4. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

There are 6 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol

328:          uint256 _setTokenAmount,

329:          IERC20 _component,

330:          bool _isEquity

340:          uint256 _setTokenAmount,

341:          IERC20 _component,

342:          bool _isEquity

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

5. `require()`/`revert()` statements should have descriptive reason strings

There are 4 instances of this issue:

File: notional-wrapped-fcash/contracts/wfCashERC4626.sol   #1

42:           require(pvExternal >= 0);

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

File: notional-wrapped-fcash/contracts/wfCashERC4626.sol   #2

245:          require(int256(type(int88).min) <= y);

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #3

129           require(
130               ac.hasDebt == 0x00 &&
131               assets.length == 1 &&
132               EncodeDecode.encodeERC1155Id(
133                   assets[0].currencyId,
134                   assets[0].maturity,
135                   assets[0].assetType
136               ) == fCashID
137:          );

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #4

316:          require(x <= uint256(type(uint88).max));

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

6. `public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There is 1 instance of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

210:      function redeemMaturedPositions(ISetToken _setToken) public nonReentrant onlyValidAndInitializedSet(_setToken) {

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

7. `constant`s should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #1

/// @audit 0x00
130:              ac.hasDebt == 0x00 &&

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

8. Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There is 1 instance of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

/// @audit address(_sendToken)
169:          require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");

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

9. Use a more recent version of solidity

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

There are 2 instances of this issue:

File: notional-wrapped-fcash/contracts/wfCashBase.sol   #1

2:    pragma solidity 0.8.11;

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

File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol   #2

2:    pragma solidity 0.8.11;

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

10. Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

There are 2 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

19:   pragma solidity 0.6.10;

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #2

2:    pragma solidity 0.8.11;

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

11. Non-library/interface files should use fixed compiler versions, not floating ones

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/wfCashERC4626.sol   #1

2:    pragma solidity ^0.8.0;

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

12. Typos

There are 4 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

/// @audit Alo
416:       * @dev Alo adjust the components / position of the set token accordingly

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

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #2

/// @audit Alo
455:       * @dev Alo adjust the components / position of the set token accordingly

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

File: notional-wrapped-fcash/contracts/wfCashBase.sol   #3

/// @audit idd
97:       /// @notice Returns the components of the fCash idd

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

File: notional-wrapped-fcash/contracts/wfCashLogic.sol   #4

/// @audit batchBalanceActionWithTrades
65:           // "batchLend" we will use "batchBalanceActionWithTrades". The difference is that "batchLend"

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

13. File is missing NatSpec

There is 1 instance of this issue:

File: index-coop-notional-trade-module/contracts/protocol/integration/wrap/notional/WrappedfCashFactory.sol (various lines)   #1

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/integration/wrap/notional/WrappedfCashFactory.sol

14. NatSpec is incomplete

There are 4 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #1

/// @audit Missing: '@param _weth'
126       /**
127        * @dev Instantiate addresses
128        * @param _controller                       Address of controller contract
129        * @param _wrappedfCashFactory              Address of fCash wrapper factory used to check and deploy wrappers
130        */
131       constructor(
132           IController _controller,
133           IWrappedfCashFactory _wrappedfCashFactory,
134           IERC20 _weth
135   
136       )
137           public
138:          ModuleBase(_controller)

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

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #2

/// @audit Missing: '@return'
154        * @param _maxSendAmount              Maximum amount to spend
155        */
156       function mintFCashPosition(
157           ISetToken _setToken,
158           uint16 _currencyId,
159           uint40 _maturity,
160           uint256 _mintAmount,
161           address _sendToken,
162           uint256 _maxSendAmount
163       )
164           external
165           nonReentrant
166           onlyManagerAndValidSet(_setToken)
167:          returns(uint256)

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

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #3

/// @audit Missing: '@return'
183        * @param _minReceiveAmount           Minimum amount of receive token to receive
184        */
185       function redeemFCashPosition(
186           ISetToken _setToken,
187           uint16 _currencyId,
188           uint40 _maturity,
189           uint256 _redeemAmount,
190           address _receiveToken,
191           uint256 _minReceiveAmount
192       )
193           external
194           nonReentrant
195           onlyManagerAndValidSet(_setToken)
196:          returns(uint256)

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

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol   #4

/// @audit Missing: '@return'
353        * @param _setToken             Instance of the SetToken
354        */
355       function getFCashPositions(ISetToken _setToken)
356       external
357       view
358:      returns(address[] memory positions)

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

15. Event is missing `indexed` fields

Each event should use three indexed fields if there are three or more fields

There is 1 instance of this issue:

File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol   #1

15:       event WrapperDeployed(uint16 currencyId, uint40 maturity, address wrapper);

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

16. Not using the named return variables anywhere in the function is confusing

Consider changing the variable to be an unnamed one

There are 6 instances of this issue:

File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol

/// @audit positions
355       function getFCashPositions(ISetToken _setToken)
356       external
357       view
358:      returns(address[] memory positions)

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

File: notional-wrapped-fcash/contracts/wfCashBase.sol

/// @audit assetToken
137:      function getAssetToken() public view override returns (IERC20 assetToken, int256 underlyingPrecision, TokenType tokenType) {

/// @audit underlyingPrecision
137:      function getAssetToken() public view override returns (IERC20 assetToken, int256 underlyingPrecision, TokenType tokenType) {

/// @audit tokenType
137:      function getAssetToken() public view override returns (IERC20 assetToken, int256 underlyingPrecision, TokenType tokenType) {

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

File: notional-wrapped-fcash/contracts/wfCashERC4626.sol

/// @audit shares
52:       function convertToShares(uint256 assets) public view override returns (uint256 shares) {

/// @audit assets
64:       function convertToAssets(uint256 shares) public view override returns (uint256 assets) {

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

code-423n4 / 2022-06-notional-coop-findings

QA Report #112

Summary

Low Risk Issues

Non-critical Issues

Low Risk Issues

1. Leap years may cause problems requiring upgrades

2. `initialize()` function does not use the `initializer` modifier

3. `safeApprove()` is deprecated

4. Missing checks for `address(0x0)` when assigning values to `address` state variables

Non-critical Issues

1. Missing `initializer` modifier on constructor

2. Missing `initializer` modifier

3. Contract implements interface without extending the interface

4. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

5. `require()`/`revert()` statements should have descriptive reason strings

6. `public` functions not called by the contract should be declared `external` instead

7. `constant`s should be defined rather than using magic numbers

8. Redundant cast

9. Use a more recent version of solidity

10. Use a more recent version of solidity

11. Non-library/interface files should use fixed compiler versions, not floating ones

12. Typos

13. File is missing NatSpec

14. NatSpec is incomplete

15. Event is missing `indexed` fields

16. Not using the named return variables anywhere in the function is confusing

code-423n4 / 2022-06-notional-coop-findings

QA Report #112

Summary

Low Risk Issues

Non-critical Issues

Low Risk Issues

1. Leap years may cause problems requiring upgrades

2. initialize() function does not use the initializer modifier

3. safeApprove() is deprecated

4. Missing checks for address(0x0) when assigning values to address state variables

Non-critical Issues

1. Missing initializer modifier on constructor

2. Missing initializer modifier

3. Contract implements interface without extending the interface

4. override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

5. require()/revert() statements should have descriptive reason strings

6. public functions not called by the contract should be declared external instead

7. constants should be defined rather than using magic numbers

8. Redundant cast

9. Use a more recent version of solidity

10. Use a more recent version of solidity

11. Non-library/interface files should use fixed compiler versions, not floating ones

12. Typos

13. File is missing NatSpec

14. NatSpec is incomplete

15. Event is missing indexed fields

16. Not using the named return variables anywhere in the function is confusing

2. `initialize()` function does not use the `initializer` modifier

3. `safeApprove()` is deprecated

4. Missing checks for `address(0x0)` when assigning values to `address` state variables

1. Missing `initializer` modifier on constructor

2. Missing `initializer` modifier

4. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

5. `require()`/`revert()` statements should have descriptive reason strings

6. `public` functions not called by the contract should be declared `external` instead

7. `constant`s should be defined rather than using magic numbers

15. Event is missing `indexed` fields