Notional x Index Coop - 0x29A's results

QA

Non-critical

[N-01] MISSING ERROR MESSAGES IN REQUIRE STATMENTS

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

L42: require(pvExternal >= 0);

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

notional-wrapped-fcash/contracts/wfCashLogic.sol:

L129-L137:
    require(
        ac.hasDebt == 0x00 &&
        assets.length == 1 &&
        EncodeDecode.encodeERC1155Id(
            assets[0].currencyId,
            assets[0].maturity,
            assets[0].assetType
        ) == fCashID
    );

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

notional-wrapped-fcash/contracts/lib/DateTime.sol:

L11: require(blockTime >= Constants.QUARTER);

L17: require(time >= Constants.DAY);

notional-wrapped-fcash/contracts/lib/EncodeDecode.sol:

L30: require(currencyId <= Constants.MAX_CURRENCIES);
L31: require(maturity <= type(uint40).max);
L32: require(assetType <= Constants.MAX_LIQUIDITY_TOKEN_INDEX);

[N-02] NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address ...). Explicitly initializing it with its default value is an anti-pattern and in some cases wastes gas.

Instances include:

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

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

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

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

L519:
- uint32 minImpliedRate = 0;
+ uint32 minImpliedRate;

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

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

[N-03] THE CONTRACT USE UNLOCKED PRAGMA

notional-wrapped-fcash/contracts/wfCashERC4626.sol, L2: Lock the pragma to 0.8.11 version like the other contracts

- pragma solidity ^0.8.0;
+ pragma solidity 0.8.11;

[N-04] OLD SOLIDITY VERSION ON hardhat.config.js

The solidity version on notional-wrapped-fcash/hardhat.config.js it's older, should be the 0.8.11 instead of 0.7.6

  solidity: {
-   version: "0.7.6",
+   version: "0.8.11",
    settings: {
      optimizer: {
        enabled: true,
        runs: 200
      }
    }
  },

Low Risk

[L-01] DON'T OVERRIDE THE receive FUNCTION

• This finding can update to med risk if the wfCashERC4626 deploy without BeaconProxy

notional-wrapped-fcash/contracts/proxy/nBeaconProxy.sol: As long as the wfCashERC4626 is used as an implementation of the nBeaconProxy it would be correct, but somebody can deploy the wfCashERC4626 without the proxy and this contract don't have the receive function Without the receive function all transfer of ether to the contract was reverted, the lines wfCashLogic.sol#L269 and wfCashLogic.sol#L292 will reverts if in the call transfer eth to the contract, breaking the withdraw and redeem functions of the wfCashERC4626 contract

Recommended Mitigation Steps: Move this function from nBeaconProxy contract to the wfCashLogic contract

nBeaconProxy.sol:L9-L11:
- receive() external payable override {
-     // Allow ETH transfers to succeed
- }

wfCashLogic.sol:L314:
+ receive() external payable override {
+     // Allow ETH transfers to succeed
+ }

[L-02] DON'T INCLUDE UNUSED FUNCTIONS

notional-wrapped-fcash/contracts/wfCashERC4626.sol, L243-L247: The function _safeNegInt88 is unused, it's best practice to remove these. It will also save gas.

[L-03] WrapperDeployed EVENT IS'T INDEXED

The emitted event is't indexed, making off-chain scripts such as front-ends of dApps difficult to filter the events efficiently.

Recommended Mitigation Steps: Add the indexed keyword in currencyId and wrapper parameters of the event

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

[L-04] OLD AND MISSING DEPENDENCIES

The notional-wrapped-fcash/package.json, L14 have an old version of @openzeppelin/contracts and don't have the @openzeppelin/contracts-upgradeable dependency, but in the notional-wrapped-fcash/brownie-config.yml, L28-L29 use OpenZeppelin/openzeppelin-contracts@4.5.0 and OpenZeppelin/openzeppelin-contracts-upgradeable@4.5.2 dependencies Add the @openzeppelin/contracts and @openzeppelin/contracts-upgradeable last versions to the notional-wrapped-fcash/package.json:

  "devDependencies": {
    "@nomiclabs/hardhat-etherscan": "^2.1.6",
-   "@openzeppelin/contracts": "^3.4.2-solc-0.7",
+   "@openzeppelin/contracts": "^4.5.0",
+   "@openzeppelin/contracts-upgradeable": "^4.5.2",
    "hardhat": "^2.6.7"
  },

• I recommend the 4.6.0 version but for work with brownie-config.yml, I put the 4.5.0 and the 4.5.2 as minimum

Also, the import of notional-wrapped-fcash/contracts/wfCashBase.sol, L14 it's wrong:

- import "@openzeppelin-upgradeable/contracts/token/ERC777/ERC777Upgradeable.sol";
+ import "@openzeppelin/contracts-upgradeable/token/ERC777/ERC777Upgradeable.sol";

Gas optimizations

[G-01] USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS TO SAVE GAS

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

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

L37: require(cashGroup.maxMarketIndex > 0, "Invalid currency");

L40-L43: 
require(
    DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp),
    "Invalid maturity"
);

notional-wrapped-fcash/contracts/wfCashLogic.sol:

L57: require(!hasMatured(), "fCash matured");

L116-L123:
    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"
    );

L129-L137:
    require(
        ac.hasDebt == 0x00 &&
        assets.length == 1 &&
        EncodeDecode.encodeERC1155Id(
            assets[0].currencyId,
            assets[0].maturity,
            assets[0].assetType
        ) == fCashID
    );

L211: require(opts.receiver != address(0), "Receiver is zero address");

L225: require(0 < cashBalance, "Negative Cash Balance");

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

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

L23: require(underlyingExternal > 0, "Must Settle");

L42: require(pvExternal >= 0);

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

[G-02] REQUIRE INSTEAD OF &&

IMPACT

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

Instances include:

notional-wrapped-fcash/contracts/wfCashLogic.sol:

L116-L123:
    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"
    );

L129-L137:
    require(
        ac.hasDebt == 0x00 &&
        assets.length == 1 &&
        EncodeDecode.encodeERC1155Id(
            assets[0].currencyId,
            assets[0].maturity,
            assets[0].assetType
        ) == fCashID
    );

MITIGATION

Breakdown each condition in a separate require statement

• Starting from Solidity v0.8.4, though require statements should be replaced with custom errors

[G-03] UNUSED IMPORTS

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol, L23: This import its unsed.

import { IERC777 } from "@openzeppelin/contracts/token/ERC777/IERC777.sol";

Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

[G-04] CACHING LENGTH ON FOR

Caching the array length is more gas efficient

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol, L238-L240: From:

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

To:

uint256 modulesLength = modules.length;

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

index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol, L254-L258: From:

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

To:

uint256 modulesLength = modules.length;

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

[G-05] CONSIDER MAKING SOME CONSTANTS AS NON-PUBLIC TO SAVE GAS

Reducing from public to private or internal can save gas when a constant isn’t used outside of its contract. I suggest changing the visibility from public to internal or private here:

notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol, L12:

- bytes32 public constant SALT = 0;
+ bytes32 private constant SALT = 0;

[G-06] asset() SHOULD GET CACHED

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

Instances include:

notional-wrapped-fcash/contracts/wfCashERC4626.sol, L212, L219:

function redeem(
    uint256 shares,
    address receiver,
    address owner
) public override returns (uint256) {
    // It is more accurate and gas efficient to check the balance of the
    // receiver here than rely on the previewRedeem method.
-   uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);
+   IERC20 _asset = IERC20(asset());
+   uint256 balanceBefore = _asset.balanceOf(receiver);

    if (msg.sender != owner) {
        _spendAllowance(owner, msg.sender, shares);
    }
    _redeemInternal(shares, receiver, owner);

-   uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);
+   uint256 balanceAfter = _asset.balanceOf(receiver);
    uint256 assets = balanceAfter - balanceBefore;
    emit Withdraw(msg.sender, receiver, owner, assets, shares);
    return assets;
}

[G-07] THE EVENT PARAMETER MIGHT NOT BE INDEXED

The emitted indexed event gives you the ability to filter events efficiently, but filtering on a boolean isn't very useful

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

L57: bool indexed _anySetAllowed
L67: bool indexed _added

To:

L57: bool _anySetAllowed
L67: bool _added

[G-08] DON'T CALCULATE THE byteCode TWICE

notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol, L26-L37: In The deployWrapper function save the byteCode in a memory var instead of calculate twice Also add name of return address ans use it in the function to short the code an improve the clarify of the code

From:

function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
    address _computedWrapper = computeAddress(currencyId, maturity);

    if (Address.isContract(_computedWrapper)) {
        // If wrapper has already been deployed then just return it's address
        return _computedWrapper;
    } else {
        address wrapper = Create2.deploy(0, SALT, _getByteCode(currencyId, maturity));
        emit WrapperDeployed(currencyId, maturity, wrapper);
        return wrapper;
    }
}

To:

function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address wrapper) {
    bytes memory byteCode = _getByteCode(currencyId, maturity);
    wrapper = Create2.computeAddress(SALT, keccak256(byteCode));

    // If wrapper has already been deployed then just return it's address
    if (! Address.isContract(wrapper)) {
        Create2.deploy(0, SALT, byteCode);
        emit WrapperDeployed(currencyId, maturity, wrapper);
    }
}