Notional x Index Coop - Funen's results

1. Title : using pragma abicoder V2

Since wfCashLogic.sol was using pragma experimental ABIEncoderV2 and used pragma ^0.7.6, it because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2 instead since Solidity 0.7.4.

Tools

Manual Review

Recommended Mitigation

you can change it into pragma abicoder v2;

2. The way to use ABIEncoderV2

The standart used for pragma ABIEncoder for pragma solidity 0.6.10;

pragma experimental ABIEncoderV2;

Since it was good to use for usually for good readibility and code as well.

Tool Used

Manual Review

3. Title : unclear comment becoming confusing

1.) File : wfCashLogic.sol Line.98

Since this was only used in function _mintInternal() and unclear what was operatorAck does to be used since cause it was one comment, that means operator Acknowledge (operatorAck). This was uncleared as an information. what was operatorAck does. if would be not do an operatorAck, comment can be deleted instead.

2.) File : NotionalTradeModule.sol

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

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

This Alo was confusing since didn't know what it is stand for. It is typo? or it can be cleared as well.

Tool Used

Manual Review

4. Title : Avoid floating pragmas: the version should be locked

The pragma declared at wfCashERC4626.sol was used ^0.8.0. As the compiler can be use as 0.8.11 and consider locking at this version the same as another.

Tool Used

Manual Review

5. Title : Related data should be grouped in a struct

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

The following maps , it can be grouped in structs.

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

Tool Used

Manual Review

6. Title : Simplify return

returns a value to the main program when exiting a function. You can do further operations on the returned value. This implementation below can be used to simplify code and good readibility.

1.) File : contracts/protocol/modules/v1/NotionalTradeModule.sol Line 172

        return _mintFCashPosition(_setToken, wrappedfCash, IERC20(_sendToken), _mintAmount, _maxSendAmount);

into :

        return _mintFCashPosition;

2.) File : contracts/protocol/modules/v1/NotionalTradeModule.sol Line 201

        return _redeemFCashPosition(_setToken, wrappedfCash, IERC20(_receiveToken), _redeemAmount, _minReceiveAmount);

into :

        return _redeemFCashPosition;

7. Title : Typo Comment

1.) File : contracts/protocol/modules/v1/NotionalTradeModule.sol Line 215

MNGER change to MANAGER

8. Title : Require Statement missing Reason String

1.) File : contracts/wfCashERC4626.sol Line. 42

        require(pvExternal >= 0);

2.) File : contracts/wfCashERC4626.sol Line. 245

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

3.) File : contracts/wfCashLogic.sol Line. 316

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

9. Title : Consider make the constracts pausable

There are many external risks so the suggestion was it should be consider making the contracts pausable, so if in the case of an unexpected event, the admin can pause transfers.

Tool Used

Manual Review

##POC https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

Recommended Mitigation Steps

Consider making contracts Pausable

1. Title : Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

Tool Used

Manual Review

Occurances

contracts/protocol/modules/v1/NotionalTradeModule.sol#L169        "Send token must be an index component"
contracts/protocol/modules/v1/NotionalTradeModule.sol#L199        "FCash to redeem must be an index component"
contracts/protocol/modules/v1/NotionalTradeModule.sol#L378        "WrappedfCash not deployed for given parameters"
contracts/protocol/modules/v1/NotionalTradeModule.sol#L485       "Not enough received amount"
contracts/protocol/modules/v1/NotionalTradeModule.sol#L573        "Token is neither asset nor underlying token"

2. Title : Using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++ .

Tools Used

Manual Review

Occurances

contracts/protocol/modules/v1/NotionalTradeModule.sol#L238        for(uint256 i = 0; i < modules.length; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L254        for(uint256 i = 0; i < modules.length; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L393         for(uint256 i = 0; i < positionsLength; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L605         for(uint256 i = 0; i < positionsLength; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L618         for(uint256 i = 0; i < positionsLength; i++) {

and this can be used prefix instead of used postfix

contracts/protocol/modules/v1/NotionalTradeModule.sol#L610                     numFCashPositions++;
contracts/protocol/modules/v1/NotionalTradeModule.sol#L623                     j++;

3. TItle : Change uint256 i = 0 into uint256 i for saving more gas

Using this implementation can saving more gas for each loops.

Tool Used

Manual Review

Recommended Mitigation

Change it

Occurances

contracts/protocol/modules/v1/NotionalTradeModule.sol#L238        for(uint256 i = 0; i < modules.length; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L254        for(uint256 i = 0; i < modules.length; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L393         for(uint256 i = 0; i < positionsLength; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L605         for(uint256 i = 0; i < positionsLength; i++) {
contracts/protocol/modules/v1/NotionalTradeModule.sol#L618         for(uint256 i = 0; i < positionsLength; i++) {