Notional x Index Coop - joestakey's results

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Commented Code
• Events indexing
• Event should be emitted in setters
• Function missing comments
• Non-library files should use the same compiler version
• Pragma should be fixed
• Public functions can be external
• Related data should be grouped in struct
• Require statements should have descriptive error strings
• Uint256 alias
• Check zero denominator
• Front-runnable initializer
• Immutable addresses lack zero-address check
• _mintInternal missing zero address check
• safeApprove() is deprecated
• upgradeable contract missing __gap[50] storage variable
• upgradeable variants of OpenZeppelin contracts should be used

summary

The main concerns are with the use of deprecated functions and incorrect versions of contracts.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

moduleIssueHook(ISetToken _setToken)
moduleRedeemHook(ISetToken _setToken)
componentIssueHook(ISetToken _setToken,uint256 _setTokenAmount,IERC20 _component,bool _isEquity)
componentRedeemHook(ISetToken _setToken,uint256 _setTokenAmount,IERC20 _component,bool _isEquity)
_deployWrappedfCash(uint16 _currencyId, uint40 _maturity)
_getWrappedfCash(uint16 _currencyId, uint40 _maturity)
redeemMaturedPositions(ISetToken _setToken)
_mintFCashPosition(ISetToken _setToken,IWrappedfCashComplete _fCashPosition,IERC20 _sendToken,uint256 _fCashAmount,uint256 _maxSendAmount)
_redeemFCashPosition(ISetToken _setToken,IWrappedfCashComplete _fCashPosition,IERC20 _receiveToken,uint256 _fCashAmount,uint256 _minReceiveAmount)
approve (ISetToken _setToken,IWrappedfCashComplete _fCashPosition,IERC20 _sendToken,uint256 _fCashAmount,uint256 _maxAssetAmount)
_mint(ISetToken _setToken,IWrappedfCashComplete _fCashPosition,uint256 _maxAssetAmount,uint256 _fCashAmount,bool _fromUnderlying)
_redeem(ISetToken _setToken,IWrappedfCashComplete _fCashPosition,uint256 _fCashAmount,bool _toUnderlying)
_isUnderlying(IWrappedfCashComplete _fCashPosition,IERC20 _paymentToken)
_getUnderlyingAndAssetTokens(IWrappedfCashComplete _fCashPosition)
_getFCashPositions(ISetToken _setToken)
_isWrappedFCash(address _fCashPosition)
_updateSetTokenPositions(ISetToken setToken,address sendToken,uint256 preTradeSendTokenBalance,address receiveToken,uint256 preTradeReceiveTokenBalance)

wfCashBase.sol

initialize(uint16 currencyId, uint40 maturity)

wfCashLogic.sol

onERC1155Received(address _operator,address _from,uint256 _id,uint256 _value,bytes calldata _data)
redeem(uint256 amount, RedeemOpts memory opts)
redeemToAsset(uint256 amount,address receiver,uint32 maxImpliedRate)
redeemToUnderlying(uint256 amount,address receiver,uint32 maxImpliedRate)
_burn(address from,uint256 amount,bytes memory userData,bytes memory operatorData)
_withdrawCashToAccount(uint16 currencyId,address receiver,uint88 assetInternalCashClaim,bool toUnderlying)
_sellfCash(address receiver,uint256 fCashToSell,bool toUnderlying,uint32 maxImpliedRate)

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

moduleIssueHook(ISetToken _setToken, uint256 /* _setTokenAmount */)
moduleRedeemHook(ISetToken _setToken, uint256 /* _setTokenAmount */)

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

WrappedfCashFactory.sol

event WrapperDeployed(uint16 currencyId, uint40 maturity, address wrapper)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to this event.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

setRedeemToUnderlying

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

function setRedeemToUnderlying

wfCashERC4626.sol

function _getMaturedValue()
function _getPresentValue()
function _redeemInternal()
function _safeNegInt88()

wfCashLogic.sol

function _mintInternal()
function _sendTokensToReceiver()
function _safeUint88()

WrappedfCashFactory.sol

function _getByteCode()
function deployWrapper()
function computeAddress()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Non-library files should use the same compiler version

PROBLEM

contracts within the scope should be compiled using the same compiler version.

SEVERITY

Non-Critical

PROOF OF CONCEPT

wfCashBase.sol, WrappedfCashFactory.sol and wfCashLogic.sol have the compiler version set to 0.8.11, while NotionalTradeModule has 0.6.10

TOOLS USED

Manual Analysis

MITIGATION

Use the same compiler version throughout the contracts

Public functions can be external

PROBLEM

Public functions that are not called by the contract should be declared external.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

redeemMaturedPositions(ISetToken _setToken)

wfCashBase.sol

getfCashId()
hasMatured()
getDecodedID()
getToken(bool useUnderlying)

TOOLS USED

Manual Analysis

MITIGATION

Make these functions external

Pragma should be fixed

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

wfCashERC4626.sol

pragma solidity ^0.8.0

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

mapping(ISetToken => bool) public redeemToUnderlying;
mapping(ISetToken => bool) public allowedSetTokens;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping:

struct SetToken {
  bool redeemToUnderlying;
  bool allowedSetTokens;
}

And it would be used as a state variable:

mapping(address =>  SetToken) public setTokens;

Require statements should have descriptive error strings

PROBLEM

Some require statements are missing descriptive error strings, which makes it harder to debugg when the function reverts

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

wfCashERC4626.sol

require(pvExternal >= 0)
require(int256(type(int88).min) <= y)

wfCashLogic.sol

require(ac.hasDebt == 0x00 &&assets.length == 1 &&EncodeDecode.encodeERC1155Id(assets[0].currencyId,assets[0].maturity,assets[0].assetType) == fCashID)
require(x <= uint256(type(uint88).max))

TOOLS USED

Manual Analysis

MITIGATION

Add descriptive strings to require statements

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

uint positionsLength = positions.length
uint positionsLength = positions.length
uint numFCashPositions
uint j

TOOLS USED

Manual Analysis

MITIGATION

replace uint with uint256

Check zero denominator

PROBLEM

When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashLogic.sol

uint256 assetInternalCashClaim = (uint256(cashBalance) * amount) / initialTotalSupply

TOOLS USED

Manual Analysis

MITIGATION

Before doing these computations, add a non-zero check to the variables aforementioned.

Front-runnable initializer

PROBLEM

The initialize function in wfCashBase.sol does not have any access control and is front-runnable. If it is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call, forcing the contract to be redeployed

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashBase.sol

function initialize(uint16 currencyId, uint40 maturity) external override initializer

TOOLS USED

Manual Analysis

MITIGATION

in WrappedfCashFactory.sol, ensure contract.initialize() is called in deployWrapper()

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

wrappedfCashFactory = _wrappedfCashFactory
weth = _weth

wfCashBase.sol

NotionalV2 = _notional
WETH = _weth

WrappedfCashFactory.sol

BEACON = _beacon

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for these function arguments.

_mintInternal missing zero address check

IMPACT

_mintInternal() should check that receiver is not the zero address.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashLogic.sol

function _mintInternal

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check

safeApprove() is deprecated

PROBLEM

This function is deprecated.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashBase.sol

assetToken.safeApprove(address(NotionalV2), type(uint256).max)
underlyingToken.safeApprove(address(NotionalV2), type(uint256).max)

TOOLS USED

Manual Analysis

MITIGATION

safeIncreaseAllowance() and safeDecreaseAllowance() should be used instead.

upgradeable contract missing __gap[50] storage variable

PROBLEM

Upgradeable contracts should have this variable to allow for new storage variables in future versions, see here.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashBase.sol

contract wfCashBase is ERC777Upgradeable

TOOLS USED

Manual Analysis

MITIGATION

add __gap[50]

upgradeable variants of OpenZeppelin contracts should be used

PROBLEM

Given that wfCashLogic.sol is deployed as a proxy contract, the Upgradeable variant of OpenZeppelin Contracts should be used. Otherwise, the constructor functions of wfCashLogic.sol parent contracts - here ReentrancyGuard - won't work for clone instances.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

wfCashBase.sol

contract wfCashLogic is ReentrancyGuard

TOOLS USED

Manual Analysis

MITIGATION

use the openzeppelin-upgradeable version of ReentrancyGuard

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Prefix increments
• Require instead of AND
• unchecked arithmetic

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

wfCashERC4626.sol

scope: _getMaturedValue(

• NotionalV2 is read twice

line 21
line 22

wfCashLogic.sol

scope: _mintInternal(

• WETH is read twice

line 69
line 70

scope: onERC1155Received(

• NotionalV2 is read three times

line 117
line 127
line 128

scope: _burn(

• NotionalV2 is read twice

line 221
line 224

scope: _mintInternal(

• WETH is read twice

line 308
line 309

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

wfCashLogic.sol

scope: redeem()

RedeemOpts memory opts

scope: _burn()

bytes memory userData
bytes memory operatorData

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

receivedAmount >= _minReceiveAmount
sentAmount <= _maxSendAmount

wfCashBase.sol

getMaturity() <= block.timestamp

wfCashERC4626.sol

pvExternal >= 0
int256(type(int88).min) <= y

wfCashLogic.sol

x <= uint256(type(uint88).max)

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-receivedAmount >= _minReceiveAmount;
+receivedAmount > _minReceiveAmount - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

-getMaturity() <= block.timestamp
+getMaturity() < block.timestamp

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

wrappedfCashFactory = _wrappedfCashFactory; weth = _weth

TOOLS USED

Manual Analysis

MITIGATION

Hardcode storage variables with their value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component")
require(_setToken.isComponent(address(wrappedfCash)), "FCash to redeem must be an index component")
require(allowedSetTokens[_setToken], "Not allowed SetToken")
require(_setToken.isInitializedModule(getAndValidateAdapter(DEFAULT_ISSUANCE_MODULE_NAME))
require(_setToken.isInitializedModule(address(_debtIssuanceModule))
require(controller.isSet(address(_setToken)) || allowedSetTokens[_setToken]
require(wrappedfCashAddress.isContract()
require(sentAmount <= _maxSendAmount
require(receivedAmount >= _minReceiveAmount
require(_paymentToken == assetToken

wfCashBase.sol

require(cashGroup.maxMarketIndex > 0
require(DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp)

wfCashERC4626.sol

require(underlyingExternal > 0
require(pvExternal >= 0)
require(int256(type(int88).min) <= y)

wfCashLogic.sol

require(!hasMatured()
require(msg.sender == address(NotionalV2) && _id == fCashID &&int256(_value) > 0,
require(ac.hasDebt == 0x00 &&assets.length == 1 &&EncodeDecode.encodeERC1155Id(assets[0].currencyId,assets[0].maturity,assets[0].assetType) == fCashID)
require(0 < cashBalance,
require(x <= uint256(type(uint88).max))

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in wfCashERC4626.sol:

Replace

require(pvExternal >= 0)

with

if (pvExternal >= 0) {
		revert NegativePV(pvExternal);
}

and define the custom error in the contract

error NegativePV(int256 pvExternal);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

uint256 i = 0
uint256 i = 0
uint256 i = 0
uint32 minImpliedRate = 0
uint256 i = 0
uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

i++
i++
i++
i++
numFCashPositions++
i++
j++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

wfCashLogic.sol

require(msg.sender == address(NotionalV2) && _id == fCashID &&int256(_value) > 0,
require(ac.hasDebt == 0x00 &&assets.length == 1 &&EncodeDecode.encodeERC1155Id(assets[0].currencyId,assets[0].maturity,assets[0].assetType) == fCashID)

TOOLS USED

Manual Analysis

MITIGATION

Break down the require statements that contain && into multiple require statements. Further savings can be achieved by replacing require statements altogether with custom errors

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas. This is particularly true in for loops, as it saves some gas at each iteration.

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

i++
i++
i++
i++
numFCashPositions++
i++
j++

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block