Canto Identity Protocol contest - Aymen0909's results

QA Report

Summary

Findings

1- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the zero value (address(0))

Risk : Low

Proof of Concept

Instances include:

File: SubprotocolRegistry.sol Line 66

note = ERC20(_noteContract);

File: CidNFT.sol Line 129-130

note = ERC20(_noteContract);
subprotocolRegistry = SubprotocolRegistry(_subprotocolRegistry);

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

2- Use Openzeppelin EnumerableSet.UintSet for managing SubprotocolData.active :

Risk : NON CRITICAL

In the CidNFT contract you should consider using openzeppelin's EnumerableSet.UintSet library for handling the SubprotocolData.active values, this library has the functions add, remove, contains already pre-built and thus can be used immediatly in a single line of code by the contract instead of writing many lines to do so. The use of this library will improve the readability of the code.

The library can be used as follows :

using EnumerableSet for EnumerableSet.UintSet;

struct SubprotocolData {
    /// @notice Mapping for ordered type
    mapping(uint256 => uint256) ordered;
    /// @notice Value for primary type
    uint256 primary;
    /// @notice List for active type
    EnumerableSet.UintSet active;
}

And the contract add/remove logic should be replace by the inbuilt add/remove of the library and existance checks can be perfomed by the contains method.

3- Duplicated input check statements should be refactored to a modifier :

Input check statements used multiple times inside a contract should be refactored to a modifier for better readability and also to save gas.

Risk : NON CRITICAL

Proof of Concept

The following check statement is repeated in both add and remove functions :

address cidNFTOwner = ownerOf[_cidNFTID];
if (
    cidNFTOwner != msg.sender &&
    getApproved[_cidNFTID] != msg.sender &&
    !isApprovedForAll[cidNFTOwner][msg.sender]
) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);

Because this check is done on the caller (msg.sender) it can be placed at the beginning of the function and thus can be refactored into a modifier, it should be replaced by a isAllowed modifier as follow :

modifier isAllowed(uint256 _cidNFTID){
    address cidNFTOwner = ownerOf[_cidNFTID];
    if (
        cidNFTOwner != msg.sender &&
        getApproved[_cidNFTID] != msg.sender &&
        !isApprovedForAll[cidNFTOwner][msg.sender]
    ) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);
    _;
}

4- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: CidNFT.sol Line 17

uint256 public constant CID_FEE_BPS = 1_000;

File: CidNFT.sol Line 191

uint256 cidFee = (subprotocolFee * CID_FEE_BPS) / 10_000;

File: SubprotocolRegistry.sol Line 17

uint256 public constant REGISTER_FEE = 100 * 10**18;

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- Optimize the register function (saves ~1800 Gas) :

The function register in the SubprotocolRegistry can be optimized to save ~1800 Gas (as shown in the table below) on each call by modifying the following :

• Place the input check statements at the beginning of the function.
• Save subprotocolData data directly to storage instead of using memory first.

Savings :

Thus the function should be modified as follow :

function register(
    bool _ordered,
    bool _primary,
    bool _active,
    address _nftAddress,
    string calldata _name,
    uint96 _fee
) external {
    if (!(_ordered || _primary || _active)) revert NoTypeSpecified(_name);
    if (
        !ERC721(_nftAddress).supportsInterface(
            type(CidSubprotocolNFT).interfaceId
        )
    ) revert NotASubprotocolNFT(_nftAddress);
    SubprotocolData storage subprotocolData = subprotocols[_name];
    if (subprotocolData.owner != address(0))
        revert SubprotocolAlreadyExists(_name, subprotocolData.owner);
    SafeTransferLib.safeTransferFrom(
        note,
        msg.sender,
        cidFeeWallet,
        REGISTER_FEE
    );
    subprotocolData.owner = msg.sender;
    subprotocolData.fee = _fee;
    subprotocolData.nftAddress = _nftAddress;
    subprotocolData.ordered = _ordered;
    subprotocolData.primary = _primary;
    subprotocolData.active = _active;
    emit SubprotocolRegistered(
        msg.sender,
        _name,
        _nftAddress,
        _ordered,
        _primary,
        _active,
        _fee
    );
}

2- Use unchecked blocks to save gas (saves ~40 gas) :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 2 instances of this issue:

File: CidNFT.sol Line 191

uint256 cidFee = (subprotocolFee * CID_FEE_BPS) / 10_000;

The above operation cannot overflow 2256 because the value of subprotocolFee is always less than 296 and CID_FEE_BPS == 1000, so the operation should be marked as unchecked.

File: CidNFT.sol Line 225

activeData.positions[_nftIDToAdd] = lengthBeforeAddition + 1;

The above operation cannot realistically overflow as this whould lead to a very big tokens array (which is not really possible), so the operation should be marked as unchecked.

3- Duplicated input check statements should be refactored to a modifier (saves ~400 gas) :

The following check statement is repeated in both add and remove functions :

address cidNFTOwner = ownerOf[_cidNFTID];
if (
    cidNFTOwner != msg.sender &&
    getApproved[_cidNFTID] != msg.sender &&
    !isApprovedForAll[cidNFTOwner][msg.sender]
) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);

Because this check is done on the function caller (msg.sender) it can be placed at the beginning of the function and thus can be refactored into a modifier to save gas, it should be replaced by a isAllowed modifier as follow :

modifier isAllowed(uint256 _cidNFTID){
    address cidNFTOwner = ownerOf[_cidNFTID];
    if (
        cidNFTOwner != msg.sender &&
        getApproved[_cidNFTID] != msg.sender &&
        !isApprovedForAll[cidNFTOwner][msg.sender]
    ) revert NotAuthorizedForCIDNFT(msg.sender, _cidNFTID, cidNFTOwner);
    _;
}

Savings (saves in total ~400 gas) :

• add function (saves ~300 gas) :

• remove function (saves ~100 gas) :

4- public functions not called by the contract should be declared external instead :

The public functions that are not called inside the contract should be declared external instead to save gas.

There is 1 instance of this issue:

File: CidNFT.sol Line 136

function tokenURI(uint256 _id) public view override returns (string memory)

5- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There is 1 instance of this issue:

File: CidNFT.sol Line 150

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