Canto Identity Protocol contest - 0xAgro's results

QA Report

Finding Summary

Low Severity

1. Input Sanitization

Non-Critical

1. Use of Exponentiation Over Scientific Notation
2. Order of Functions Not Compliant With Solidity Docs
3. Long Lines
4. Inconsistent Named Returns
5. Spelling Mistakes
6. Bulky Functions
7. Consider Gender Neutral Terminology
8. Missing Max Fee Error Message
9. Inconsistent Trailing Period

Low Severity

1. Input Sanitization

The add function in CidNFT.sol takes in an argument called _key. _key is only used in the add function if _type == AssociationType.ORDERED. To prevent possible user errors when calling the add function, consider adding a require statement forcing _key to be empty if _type != AssociationType.ORDERED.

Non-Critical

1. Use of Exponentiation Over Scientific Notation

For better style and less computation consider replacing any power of 10 exponentiation (10**3) with its equivalent scientific notation (1e3). For code clarity it is understood why 100 * 10**18 is used. This issue is specifically about the 10**18.

/src/SubprotocolRegistry.sol

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

Suggested Change

17:	uint256 public constant REGISTER_FEE = 100 * 1e18;

2. Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

• CidNFT.sol: external functions are positioned after public functions.

3. Long Lines

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

The following lines are longer than 120 characters, it is suggested to shorten these lines:

/src/CidNFT.sol

• 148, 162, 185, 258, 261, 327.

/src/SubprotocolRegistry.sol

• 71, 72, 78.

4. Inconsistent Named Returns

Some functions use named returns and others do not. It is best for code clearity to keep a consistent style.

1. The following contracts only have named returns (EX. returns(uint256 foo)): AddressRegistry.sol.
2. The following contracts only have non-named returns (EX. returns(uint256)): SubprotocolRegistry.sol.
3. The following contracts have both: CidNFT.sol.

5. Spelling Mistakes

There are some spelling mistakes throughout the codebase. Consider fixing all spelling mistakes.

/src/CidNFT.sol

• The word non-existent is misspelled as non-existing.
• The word non-existent is misspelled as non-existant.
• The word subprotocol is misspelled as subprotocl (1).
• The word subprotocol is misspelled as subprotocl (2).

6. Bulky Functions

The core functions of the codebase (add and remove of CidNFT.sol) are bulky and relatively hard to read as a result. Consider breaking the functions up into sub-functions (perhaps by association type).

7. Consider Gender Neutral Terminology

Permissionless software implies that anyone of any background that has access may use the system. Consider using as broad of language as possible when talking about users. Even if most are, probabilistically, not all users will be male.

/src/CidNFT.sol

_mint(msg.sender, ++numMinted); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back

8. Missing Max Fee Error Message

When registering a new subprotocol in register of SubprotocolRegistry.sol the max value is not checked. Although the function will revert if _fee exceeds the size of a uint96, no proper error message will trigger. Consider adding an error message to better indicate the max fee size.

9. Inconsistent Trailing Period

At the start of CidNFT.sol most comments do not have a trailing . (ex) unless followed by another sentence (ex). Consider sticking to the original style. The following cases void the original style:

/src/CidNFT.sol

• Followed by a sentence, but no trailing .: L69, L162, L185, L234, L235, L258, L294, L306, L318.
• Single sentence with trailing .: L145, L146, L161.

/src/SubprotocolRegistry.sol

• Followed by a sentence, but no trailing .: L31, L77, L78, L105.

/src/AddressRegistry.sol

• Followed by a sentence, but no trailing .: L40, L61.

Gas Report

Finding Summary

1. Reversion Can Be Done Earlier (-73726, -9.434%)
2. Convert To AND and Split Reversion (-546, -0.057%)
3. uint256 Iterator Checked Each Iteration (-315, -0.102%)

1. Reversion Can Be Done Earlier

Error checks should revert as early as possible to prevent unnecessary computation.

/src/CidNFT.sol

• On L183 _nftIDToAdd == 0 is checked. _nftIDToAdd is a parameter and therefore can be checked at the very beginning of add.

/src/SubprotocolRegistry.sol

• On L88 !(_ordered || _primary || _active) is checked. All three variables are parameters and therefore can be checked at the very beginning of register.
• On L93 !ERC721(_nftAddress).supportsInterface(type(CidSubprotocolNFT).interfaceId) is checked. _nftAddress is a parameter and therefore can be checked at the very beginning of register (please consider possible security ramifications).

2. Convert To AND and Split Reversion

L88 can be converted to an AND statement and transformed as follows to save gas:

From

88:	if (!(_ordered || _primary || _active)) revert NoTypeSpecified(_name);

To

if (!_ordered) {
    if (!_primary) {
        if (!_active) revert NoTypeSpecified(_name);
    }
}

3. uint256 Iterator Checked Each Iteration

A uint256 iterator will not overflow before the check variable overflows. Unchecking the iterator increment saves gas.

Example

//From
for (uint256 i; i < len; ++i) {
	//Do Something
}
//To
for (uint256 i; i < len;) {
	//Do Something
	unchecked { ++i; }
}

/src/CidNFT.sol

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