Notional x Index Coop - c3phas's results

Natspec is incomplete

File: NotionalTradeModule.sol line 175

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

Missing @return

File: NotionalTradeModule.sol line 352-358

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

Missing @return

TYPOS

File: NotionalTradeModule.sol line 215

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

MANGER instead of MANAGER

File: NotionalTradeModule.sol line 416

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

Alo Instead of Also

File: NotionalTradeModule.sol line 455

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

Alo Instead of Also

File: NotionalTradeModule.sol line 111

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

wether instead of Whether

Lack of consistency in using uint256 and uint

some variables have been declared using uint while others are explicitly using uint256. I suggest sticking to one as they both mean the same thing Function _getFCashPositions uses both uint and uint256 and also majority of the other variables in the contract are using uint256

line 602-603

        uint positionsLength = positions.length;
        uint numFCashPositions;

Then inside the loop within this function we use uint256

        for(uint256 i = 0; i < positionsLength; i++) {

Also outside this function most of the other variables are using uint256

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File: wfCashERC4626.sol line 2

pragma solidity ^0.8.0;

FINDINGS

use shorter revert strings(less than 32 bytes)

You can (and should) attach error reason strings along with require statements to make it easier to understand why a contract call reverted. These strings, however, take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

File: NotionalTradeModule.sol line 169

require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");

File: NotionalTradeModule.sol line 199

require(_setToken.isComponent(address(wrappedfCash)), "FCash to redeem must be an index component");

File: NotionalTradeModule.sol line 378

require(wrappedfCashAddress.isContract(), "WrappedfCash not deployed for given parameters");

File: NotionalTradeModule.sol line 573

            require(_paymentToken == assetToken, "Token is neither asset nor underlying token");

Cache the length of arrays in loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration. Therefore, it’s possible to save a significant amount of gas especially when the length is significantly big.

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: NotionalTradeModule.sol line 237-239

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

The above should be changed to

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

Other instances to modify File: NotionalTradeModule.sol line 253-254

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

No need to initialize variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File: NotionalTradeModule.sol line 519

        uint32 minImpliedRate = 0;

File: NotionalTradeModule.sol line 238

for(uint256 i = 0; i < modules.length; i++) {

Since i is a uint256, it has a default value of 0 so we don't need to initialize it here The above should be modified to

for(uint256 i; i < modules.length; i++) {

Other instances to modify File: NotionalTradeModule.sol line 254

        for(uint256 i = 0; i < modules.length; i++) {

File: NotionalTradeModule.sol line 393

for(uint256 i = 0; i < positionsLength; i++) {

File: NotionalTradeModule.sol line 605

        for(uint256 i = 0; i < positionsLength; i++) {

File: NotionalTradeModule.sol line 618

Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File: wfCashLogic.sol line 116-123

        require(
            msg.sender == address(NotionalV2) &&
            // Only accept the fcash id that corresponds to the listed currency and maturity
            _id == fCashID &&
            // Protect against signed value underflows
            int256(_value) > 0,
            "Invalid"
        );

The above should be modified to

require (msg.sender == address(NotionalV2),"Invalid");
require (_id == fCashID ,"Invalid");
require(int256(_value) > 0,"Invalid");

Proof The following tests were carried out in remix with both optimization turned on and off

function multiple (uint a) public pure returns (uint){
	require ( a > 1 && a < 5, "Initialized");
	return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

function multiple(uint a) public pure returns (uint){
	require (a > 1 ,"Initialized");
	require (a < 5 , "Initialized");
	return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

File:NotionalTradeModule.sol line 238

        for(uint256 i = 0; i < modules.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Other instances

File:NotionalTradeModule.sol line 254

        for(uint256 i = 0; i < modules.length; i++) {

File:NotionalTradeModule.sol line 393

        for(uint256 i = 0; i < positionsLength; i++) {

File:NotionalTradeModule.sol line 605

        for(uint256 i = 0; i < positionsLength; i++) {

File:NotionalTradeModule.sol line 618

        for(uint256 i = 0; i < positionsLength; i++) {