Canto Identity Subprotocols contest - lukris02's results

QA Report for Canto Identity Subprotocols contest

Overview

During the audit, 8 low and 9 non-critical issues were found.

Low Risk Findings(8)

L-1. Docs may mislead users

Description

Docs says: "A fusing fee that is proportial to the length of the name is charged". Although it seems obvious that a shorter name is more unique and therefore more expensive. Reading the protocol documentation, the user may think that the longer the name, the higher the price and, therefore, they may choose a shorter name and pay more.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L113
• https://github.com/code-423n4/2023-03-canto-identity/tree/main/canto-namespace-protocol#fusing

Recommendation

Change to "A fusing fee that is inversely proportional to the length of the name is charged" or "A fusing fee that is proportional to the length of the name is charged. The shorter the name, the higher the fee".

L-2. Return the amount available for minting

Description

For convenience, the amount available for minting should be returned in the error message.

Instances

• if (startingTrayId + _amount - 1 > PRE_LAUNCH_MINT_CAP) revert MintExceedsPreLaunchAmount();

Recommendation

Change to:

if (startingTrayId + _amount - 1 > PRE_LAUNCH_MINT_CAP) revert MintExceedsPreLaunchAmount(PRE_LAUNCH_MINT_CAP - startingTrayId - 1);

L-3. Add comment about _safeMint

Description

If the comment in the Tray.sol is also valid for ProfilePicture.sol, then duplicate the comment in the ProfilePicture.sol. Otherwise, _safeMint can be used.

Instances

• _mint(msg.sender, tokenId);
• _mint(msg.sender, _amount); // We do not use _safeMint on purpose here to disallow callbacks and save gas

L-4. Use the two-step-transfer of ownership

Description

"{Owned} from "solmate/auth/Owned.sol" has one-step transfer of ownership. If the owner accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L5
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L7

Recommendation

Consider using a two-step-transfer of ownership: the current owner would nominate a new owner, and to become the new owner, the nominated account would have to approve the change, so that the address is proven to be valid.

L-5. Critical Address Changes Should Use Two-step Procedure

Description

Lack of two-step procedure for critical operations leaves them error-prone.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L196
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L204
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L210
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L218

Recommendation

Consider adding two step procedure on the critical functions.

L-6. Use Checks Effects Interactions pattern

Description

For reducing the attack surface for malicious contracts trying to hijack control flow after an external call, it is best practice to use this pattern.

Instances

• Line80-82:

        uint256 tokenId = ++numMinted;
        if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
            revert PFPNotOwnedByCaller(msg.sender, _nftContract, _nftID);

Recommendation

Change to:

        if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
            revert PFPNotOwnedByCaller(msg.sender, _nftContract, _nftID);
        uint256 tokenId = ++numMinted;

L-7. Malicious token contracts can pass the ownership check

Description

Verification that the user owns NFT can be passed using malicious token contracts, this can negatively affect the operation of the protocol.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L81-L82
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L101

L-8. Might need to change mint() function in ProfilePicture.sol

Description

The user may mint many PFP NFT with the same reference _nftContract and _nftID.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L79-L88

Recommendation

If it is not intended that the user can mint multiple PFP NFTs using the same reference NFT, then it is necessary to keep track of which NFTs the user has already used.

Non-Critical Risk Findings(9)

NC-1. Use gender-neutral pronouns

Instances

• Any user can mint a Bio NFT by calling Bio.mint and passing his biography.
• A user can therefore precompute which trays he will get.

Recommendation

Change to:

• Any user can mint a Bio NFT by calling Bio.mint and passing their biography.
• A user can therefore precompute which trays they will get.

NC-2. Validate tray price

Description

Price validation can be added.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L106

Recommendation

Consider setting a minimum and maximum allowable price per tray and check if the function parameter matches it.

NC-3. Order of Functions

Description

According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:

1. constructor
2. receive function (if exists)
3. fallback function (if exists)
4. external
5. public
6. internal
7. private

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L70
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L43
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L90
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L119

Recommendation

Public functions should be placed after external.

NC-4. Order of Layout

Description

According to Order of Layout, inside each contract, library or interface, use the following order:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L21
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L30
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L57

Recommendation

Place structs before state variables.

NC-5. Missing NatSpec

Description

NatSpec is missing for 2 functions in 1 contract.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L231
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L245

Recommendation

Add NatSpec for all functions.

NC-6. Commented code

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L55

Recommendation

Delete commented code.

NC-7. Inconsistency when using uint and uint256

Description

Some variables is declared as uint and some as uint256. Below 5 instances when uint is used, while uint256 is used in all other 110 instances.

Instances

• uint lengthInBytes = bioTextBytes.length;
• uint lines = (lengthInBytes - 1) / 40 + 1;
• uint bytesOffset;
• for (uint i; i < lengthInBytes; ++i) {
• for (uint i; i < lines; ++i) {

Recommendation

Stick to one style.

NC-8. Maximum line length exceeded

Description

According to Style Guide, maximum suggested line length is 120 characters. Longer lines make the code harder to read.

Instances

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L56
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L41
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L53
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L72
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L35
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L126
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L152
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L62
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L247

Recommendation

Make the lines shorter.

NC-9. Missing leading underscores

Description

Private constants, immutables, and state variables should have a leading underscore.

Instances

• ICidNFT private immutable cidNFT;
• mapping(uint256 => ProfilePictureData) private pfp;
• address private revenueAddress;
• mapping(uint256 => Tray.TileData[]) private nftCharacters;
• uint256 private constant TILES_PER_TRAY = 7;
• uint256 private constant SUM_ODDS = 109;
• uint256 private constant NUM_CHARS_EMOJIS = 420;
• uint256 private constant NUM_CHARS_LETTERS = 26;
• uint256 private constant NUM_CHARS_LETTERS_NUMBERS = 36;
• uint256 private constant PRE_LAUNCH_MINT_CAP = 1_000;
• address private revenueAddress;
• mapping(uint256 => TileData[TILES_PER_TRAY]) private tiles;
• uint256 private prelaunchMinted = type(uint256).max;

Recommendation

Add leading underscores where needed.

Gas Optimizations Report for Canto Identity Subprotocols contest

Overview

During the audit, 1 gas issue were found.
Total savings ~560.

Gas Optimizations Findings(1)

G-1. Use unchecked blocks for subtractions where underflow is impossible

Description

In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. When an overflow or underflow isn’t possible (after require or if-statement), some gas can be saved by using unchecked blocks.

Instances

• if ((i > 0 && (i + 1) % 40 == 0) || prevByteWasContinuation || i == lengthInBytes - 1) { (lengthInBytes - 1) - checked in line 49.
• if (i != lengthInBytes - 1) { checked in line 49.
• bioTextBytes[i - 2] == 0xE2 && checked in line 79.
• bioTextBytes[i - 1] == 0x80 && checked in line 79.
• uint256 fusingCosts = 2**(13 - numCharacters) * 1e18; checked in line 112.
• prelaunchMinted = _nextTokenId() - 1; safe because startTokenId = 1.
• tileData.fontClass = 3 + uint8((res - 80) / 8); safe because here res >= 80.
• tileData.fontClass = 5 + uint8((res - 96) / 4); safe because here res >= 96.
• tileData.fontClass = 7 + uint8((res - 104) / 2); safe because here res >= 104.
• supportsSkinToneModifier = _characterIndex >= EMOJIS_LE_THREE_BYTES - EMOJIS_MOD_SKIN_TONE_THREE_BYTES;
• byteOffset = EMOJIS_BYTE_OFFSET_FOUR_BYTES + (_characterIndex - EMOJIS_LE_THREE_BYTES) * 4; checked in line 83.
• supportsSkinToneModifier = _characterIndex >= EMOJIS_LE_FOUR_BYTES - EMOJIS_MOD_SKIN_TONE_FOUR_BYTES;
• byteOffset = EMOJIS_BYTE_OFFSET_SIX_BYTES + (_characterIndex - EMOJIS_LE_FOUR_BYTES) * 6; checked in line 87.
• byteOffset = EMOJIS_BYTE_OFFSET_SEVEN_BYTES + (_characterIndex - EMOJIS_LE_SIX_BYTES) * 7; checked in line 91.
• byteOffset = EMOJIS_BYTE_OFFSET_EIGHT_BYTES + (_characterIndex - EMOJIS_LE_SEVEN_BYTES) * 8; checked in line 94.
• byteOffset = EMOJIS_BYTE_OFFSET_FOURTEEN_BYTES + (_characterIndex - EMOJIS_LE_EIGHT_BYTES) * 14; checked in line 97.

Saved

This saves ~35.
So, ~35*16 = 560