Canto Identity Subprotocols contest - igingu's results

Lines of code

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

Vulnerability details

[H-02] Namespace: Fusing will only result in namespaces containing font class 0, irrelevant of fused tiles

After minting trays, a user can fuse tiles from multiple trays into a namespace. A tray tile specifies a character font class, the character's index and the character modifier. There are 10 font classes, starting from most common EMOJI (font class = 0 and probability 32/109) to most uncommon BLOCKS INVERTED (font class = 9 and probability 1/109).

Current implementation fuses ALL CHARACTERS using hardcoded fontClass 0.

Impact

Irrelevant of how rare the fused character tile is, it is always fused from the most common font class (0 = EMOJIs), effectively burning Tray NFTs of any rarity and turning them into the most common characters. This means that irrespective of how rare your Tray NFT characters are, everyone will always have names composed of only EMOJIs.

Font classes 1-9 account for (77/109)% of minted character Trays (77 = 109 (sum of all shares), minus 32 (shares for font class 0)), which means that when fusing, 77 out of 109 times you will not get the character you should have gotten, but something else, of the most common occurence and the least scarcity.

Remediation

canto-namespace-protocol/src/Namespace.sol#L144
  bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);
  ->
  bytes memory charAsBytes = Utils.characterToUnicodeBytes(tileData.fontClass, tileData.characterIndex, characterModifier);

Add a Forge unit test for every font class, making sure that fusing results in the expected Namespace string.

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L247 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L266 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L141

Vulnerability details

[M-02] Namespace protoccol: trays are minted based on very easy to calculate pseudo-randomness

In Namespace protocol, a Namespace NFT is created from fusing trays, which are NFTs holding 7 characters. Trays are minted based on an open, easy to predict pseudo-random-number-generator, implemented as iteratePRNG. Each tray can contain characters from 10 different font classes, with probabilities for each font class ranging from 32/109 to 1/109. This means that characters belonging to super rare font classes will be considerably more scarce, and so a lot more valuable.

Using a pseudo-random-number-generator means one can predict what tray they will get before minting, with a lot of ease.

Impact:

The impact is three-fold, first two being more dangerous to normal users (sniping and front-running):

• off-chain bots can monitor the state of the pseudo-random-number-generator, and snipe upcoming trays with characters from super rare font classes, effectively establishing a monopoly over them.
• off-chain bots can monitor the mempool for transactions which will end up minting characters from super rare font classes, and frontrun them, effectively establishing a monopoly over them.
• most people have a nickname/name that they use in the virtual space, which they will probably want to fuse into their NFT. For that they will need to mint enough trays so that they have all the required characters. Knowing what tray I will get before minting it would make it a waste of money if I don't get the right characters, which can make me not mint it in the first place, which will slow down adoption of this protocol.

Remediation:

Use Chainlink's Verified Random Function to generate truly random numbers. If implemented correctly, integrating Chainlink VRF won't have possibility of predicting future tray, sniping or front-running.

[L-01] ProfilePicture: Misleading comments and Error names

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L69 also reverts if ProfilePicture.sol NFT with _id is not linked to any CIDNFT, because of if (cidNFTID == 0). Error name should also change, or be split into two, one error for ProfilePicture NFT not linked, another error for nftContract.nftID NFT no longer in posession of associated CIDNFT owner.

[L-02] Literal values should be stored in constants, to greatly improve whole codebase's readability

The whole codebases uses a lot of literal values (especially Namespace protocol). These should be stored as constant values in the contracts (which don't incur additional gas cost), in order to make the code more self-explanatory => comments explaining the values could be removed, reducing maintenance time in the future.

Recommendation:

canto-bio-protocol/src/Bio.sol
    uint256 private constant CANTO_MAINNET_BLOCKID = 7700;
    uint256 private constant MAX_BIO_LENGTH = 200;
    uint256 private constant MAX_NUM_CHARS_BEFORE_NEWLINE = 40;
    string private constant HTML_BEGINNING = "<text x="50%" y="50%" dominant-baseline="middle" text-anchor="middle">";

    // would also save gas, instead of creating the in-memory variable ```string memory svg```
    string private constant SVG_HTML = "<svg xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMinYMin meet"; viewBox="0 0 400 100"><style>text { font-family: sans-serif; font-size: 12px; }</style>";

canto-namespace-protocol/src/Utils.sol
    uint256 private constant ASCII_STARTING_INDEX = 97;
    uint256 private constant ITERATE_PRNG_MULTIPLIER = 15485863;
    uint256 private constant ITERATE_PRNG_MODULO = 2038074743;

canto-namespace-protocol/src/Namespace.sol
    uint256 private constant MAX_BYTES_PER_CHARACTER = 33;
    uint256 private constant FONT_CLASS_EMOJIS = 0;

[L-03] Bio: Bio.tokenURI function is very hard to understand and has redundant check

Bio.tokenURI should have more spacing and comments, explaining what each code block does.

In (i > 0 && (i + 1) % 40 == 0), the i > 0 check is redundant since i will always be > 0 if (i + 1) % 40 == 0 and i is an unsigned integer.

Recommendation:

Refactor function as:

    function tokenURI2(uint256 _id) public view override returns (string memory) {
        if (_ownerOf[_id] == address(0)) revert TokenNotMinted(_id);

        string storage bioText = bio[_id];
        bytes memory bioTextBytes = bytes(bioText);
        uint lengthInBytes = bioTextBytes.length;

        // Insert a new line after 40 characters, taking into account unicode character
        // Note: for 39 characters, need 1 newline (after all 39)
        //       for 40 characters, need 1 newline (after all 40)
        //       for 41 characters, need 2 newlines (after first 40, and after last 1)
        uint lines = (lengthInBytes - 1) / 40 + 1;
        string[] memory strLines = new string[](lines);

        bool prevByteWasContinuation;
        uint256 insertedLines;
        // Because we do not split on zero-width joiners, line in bytes can technically be much longer. Will be shortened to the needed length afterwards
        bytes memory bytesLines = new bytes(80);
        uint bytesOffset;

        for (uint i; i < lengthInBytes - 1; ++i) {
            // put current character in resulting array
            bytes1 character = bioTextBytes[i];
            bytesLines[bytesOffset] = character;
            bytesOffset++;
            
            // Note - if am at last character before newline should come,
            //        or prevByteWasContinuation,
            //        or I reached the last character,
            //        should potentially insert line break
            if ((i > 0 && (i + 1) % 40 == 0) || prevByteWasContinuation || i == lengthInBytes - 1) {
                bytes1 nextCharacter;
                if (i != lengthInBytes - 1) {
                    nextCharacter = bioTextBytes[i + 1];
                }
                if (nextCharacter & 0xC0 == 0x80) {
                    // Unicode continuation byte, top two bits are 10
                    // Note - https://stackoverflow.com/questions/9356169/utf-8-continuation-bytes
                    prevByteWasContinuation = true;
                } else {
                    // Do not split when the prev. or next character is a zero width joiner. Otherwise, 👨‍👧‍👦 could become 👨>‍👧‍👦
                    // Furthermore, do not split when next character is skin tone modifier to avoid 🤦‍♂️\n🏻
                    if (
                        // Note that we do not need to check i < lengthInBytes - 4, because we assume that it's a valid UTF8 string and these prefixes imply that another byte follows
                        // Note - check if next character is beginning of Zero Width Joiner 
                        //         https://emojipedia.org/zero-width-joiner/#:~:text=Zero%20Width%20Joiner%20(ZWJ)%20is,invisible%20character%20when%20used%20alone.
                        (nextCharacter == 0xE2 && bioTextBytes[i + 2] == 0x80 && bioTextBytes[i + 3] == 0x8D) ||
                        // Note - check if next character is a skin modifier
                        (nextCharacter == 0xF0 &&
                            bioTextBytes[i + 2] == 0x9F &&
                            bioTextBytes[i + 3] == 0x8F &&
                            uint8(bioTextBytes[i + 4]) >= 187 &&
                            uint8(bioTextBytes[i + 4]) <= 191) ||
                        // Note - check for ending of Zero Width Joiner 
                        //        https://emojipedia.org/zero-width-joiner/#:~:text=Zero%20Width%20Joiner%20(ZWJ)%20is,invisible%20character%20when%20used%20alone.
                        (i >= 2 &&
                            bioTextBytes[i - 2] == 0xE2 &&
                            bioTextBytes[i - 1] == 0x80 &&
                            bioTextBytes[i] == 0x8D)
                    ) {
                        prevByteWasContinuation = true;
                    } else {
                        // Note - Trim down bytesLines to correct length, by changing length directly
                        assembly {
                            mstore(bytesLines, bytesOffset)
                        }

                        // Note - We have our newline
                        strLines[insertedLines++] = string(bytesLines);

                        // Note - Reset running variables
                        bytesLines = new bytes(80);
                        prevByteWasContinuation = false;
                        bytesOffset = 0;
                    }
                }
            }
        }
        
        string memory text = HTML_BEGINNING;
        for (uint i; i < lines; ++i) {
            text = string.concat(text, '<tspan x="50%" dy="20">', strLines[i], "</tspan>");
        }
        string memory json = Base64.encode(
            bytes(
                string.concat(
                    '{"name": "Bio #',
                    LibString.toString(_id),
                    '", "description": "',
                    bioText,
                    '", "image": "data:image/svg+xml;base64,',
                    Base64.encode(bytes(string.concat(SVG_HTML, text, "</text></svg>"))),
                    '"}'
                )
            )
        );
        return string(abi.encodePacked("data:application/json;base64,", json));
    }

[L-04] Namespace: Consider introducing enums for font classes, to navigate the code more easily, remove dead comments and avoid future bugs

By naming variables better and making the code more "self-explainable", one not only makes it easier to read, but also minimizes future maintainence efforts to update comments which were meant to explain the code ("dead comments").

enum FontClass { EMOJI, BASIC, SCRIPT, SCRIPT_BOLD, OLDE, OLDE_BOLD, SQUIGGLE, ZALGO, BLOCKS, BLOCKS_INVERTED };

if (_fontClass == 0) {
// Emojis
uint256 byteOffset;
->
if (_fontClass == FontClass.EMOJI) {
uint256 byteOffset;

} else if (_fontClass == 7) {
    // Zalgo
    uint8 asciiStartingIndex = 97;
->
} else if (_fontClass == FontClass.ZALGO) {
    uint8 asciiStartingIndex = 97;

[L-05] Namespace: Accessing out-of-bounds array in getTile should revert with specific error message

Trying to fuse a tile with _tileOffset >= TILES_PER_TRAY would cause a Solidity VM Panic code, but it should be gracefully handled and a suggestive error should be thrown instead.

Recommendation

    function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData) {
        if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
        tileData = tiles[_trayId][_tileOffset];
    }
    ->
    function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData) {
        if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
        if (_tileOffset >= TILES_PER_TRAY) revert TileOffsetShouldBeSmallerThanLimit(_trayId, _tileOffset);
        tileData = tiles[_trayId][_tileOffset];
    }

[L-06] Namespace: Adoption might be impacted by how fast fusing prices increase, based on number of fused tiles

The cost of fusing tiles into your Namespace NFT doubles, for each character less that you introduce => exclusivity comes from having a smaller name, not a longer one.

The doubling seems like a very steep price increase:

• 13 characters is 1 NOTE
• 12 characters is 2 NOTE ...
• 10 characters is already 8 NOTE which is ~ 8$ ...
• 3 character is 1024 NOTE ...
• 1 character is 4096 NOTE

Recommendation:

Come up with a different formula for computing fusing prices, which doesn't double every step.

[L-07] Namespace: The initial seed for minting trays is set by deployer

Ideally deployer should have less control ofer the _initHash, I don't see why they would have to set it themselves and not take it from something like block.timestamp, which wouldn't be that much in the deployer's control.

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

Recommendation:

Initialize lastHash with block.timestamp, converted to bytes32.

[N-01] For modern and more readable code; update import usages

By importing whole Solidity files, one can end up importing more than needed, which increases gas costs and also doesn't easily highlight the contracts/interfaces/libraries one actually needs.

All source files contain such imports, and need changing.

Recommendation:

canto-pfp-protocol/src/ProfilePicture.sol:5:
    import "src/interfaces/Turnstile.sol";
    ->
    import {Turnstile} from "src/interfaces/Turnstile.sol";

[N-02] ProfilePicture: Important state variables like CIDNFT address should be public

In Canto Identity, subprotocols are tightly coupled with the CIDNFT address used, which means one should be able to query the CIDNFT address easily, especially since there is no reason to "hide" it.

Recommendation:

canto-pfp-protocol/src/ProfilePicture.sol:14:
    ICidNFT private immutable cidNFT;
    ->
    ICidNFT public immutable cidNFT;

[N-03] Solidity Code Style conventions

Function arguments should use mixedCase

All source files contain function arguments starting with _ instead of using mixedCase, and need changing.

---

Functions should be grouped and ordered according to their visibility

ProfilePicture:

• ProfilePicture.mint should be at the top since it's external

Bio:

• Bio.mint should be at the top since it's external

Namespace:

• _drawing should be at the bottom since it's private
• Tray.tokenURI should be after all external functions
• Namespace.tokenURI should be after all external functions

---

Common practice is that imports are spaced out and grouped according to the SolidityLang ordering

SolidityLang Style Guide mentions the order of components in a file, and common convention is that imports are grouped and ordered the same as if they were components: Interfaces, Libraries, Contracts, for ease of readability.

All source files contain such imports, and need changing.

    import {ERC721} from "solmate/tokens/ERC721.sol";
    import {Owned} from "solmate/auth/Owned.sol";
    import {Base64} from "solady/utils/Base64.sol";
    import "./Tray.sol";
    import "./Utils.sol";
    import "../interface/Turnstile.sol";
    ->
    import "../interface/Turnstile.sol";

    import {Base64} from "solady/utils/Base64.sol";
    import "./Utils.sol";

    import {ERC721} from "solmate/tokens/ERC721.sol";
    import {Owned} from "solmate/auth/Owned.sol";
    import "./Tray.sol";

[N-04] Slither should be configured to run only on source files, while currently it runs on everything

Recommendation:

In each subprotocol folder, create a slither.config.json file with contents, filtering non-source code:

{
    "filter_paths": "(lib/|test/|node_modules/|src/test/)"
}

[N-05] Namespace: characterToUnicodeBytes for fontClass == 0 should use numBytes for calculation for improved code readability

Recommendation:

numBytes = 3;
byteOffset = _characterIndex * 3;
supportsSkinToneModifier = _characterIndex >= EMOJIS_LE_THREE_BYTES - EMOJIS_MOD_SKIN_TONE_THREE_BYTES;
->
numBytes = 3;
byteOffset = _characterIndex * numBytes;
supportsSkinToneModifier = _characterIndex >= EMOJIS_LE_THREE_BYTES - EMOJIS_MOD_SKIN_TONE_THREE_BYTES;

Change the above for all if statements (3, 4, 6, 7, 8, 14 bytes cases).

[N-06] Namespace: Use of bytes.concat() instead of abi.encodePacked()

When needing to append bytes, since Solidity compiler version 0.8.4, bytes.concat() can and should be used instead of abi.encodePacked(,).

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L104 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L110 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L115 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L149 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L158 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Utils.sol#L167

[N-07] Typos and wrong comments

asciiStartingIndex = 22; // Starting index for (lowercase) characters - 25
->
asciiStartingIndex = 22; // Starting index for digits, - 25

[N-08] NatSpec comments should be increased in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation.

Namespace, Tray, Utils and Bio contracts contain multiple complex functions which should be explained more, using the @dev NatSpec annotation. The logic around character to bytes encoding implies a lot of prior Unicode logic which is not easily made available to the reading eyes, through comments.

[N-09] Tests don't test core functionality, although coverage is at 100%

The test suite was envisioned around 100% coverage of all statements and branches, but doesn't necessarily cover some important features of the code, vital to the project's functionality:

Examples:

• should have tests for characterToUnicodeBytes, at least one test for each font class
• should have tests for characterToUnicodeBytes, at least one for each byte number of EMOJIs
• should have tests for fuse, checking that the resulting name is actually what is expected, at least one test for each font class

[N-10] Missing Events for constructors + some Events are missing important data

Smart contracts' constructors should emit all the arguments used for construction, so they could be easily read off-chain, on block explorers.

PrelaunchEnded event should emit final number of prelaunch NFTs emitted.

[N-11] Namespace: Critical address changes should be two-step procedures

Changing the $NOTE address is critical for the well-being of the ecosystem, while changing the revenueAddress is relevant for the correct receival of funds. Transforming these into two-step processes would make them less error-prone.

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

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

Recommendation:

Implement a similar technique to OpenZeppelin's Ownable2Step, where owner could request an address change, and then confirm it using a different function.

[N-12] Uppercase immutable variables

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

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L37 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L50 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L17

[N-13] Use underscores for number literals, for improved code readability

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

Recommendation:

unchecked {
    iteratedState = _currState * 15485863;
    iteratedState = (iteratedState * iteratedState * iteratedState) % 2038074743;
}
->
unchecked {
    iteratedState = _currState * 15_485_863;
    iteratedState = (iteratedState * iteratedState * iteratedState) % 2_038_074_743;
}

[S-01] Suggestion: To make Slither work on the canto-namespace-protocol folder, you need a different Slither version, like 0.9.1

https://github.com/crytic/slither/issues/1610

Recommendation:

pip3 install slither-analyzer==0.9.1
slither --version
0.9.1
slither . ### no longer throws an error

[G-01] 6/7 storage reads can be removed by storing all tiles inside one uint256

Each Tile uses 32 bits on memory:

struct TileData {
    /// @notice Allowed values between 0 (emoji) and 9 (font5 rare)
    uint8 fontClass;
    /// @notice For Emojis (font class 0) between 0..NUM_CHARS_EMOJIS - 1, otherwise between 0..NUM_CHARS_LETTERS - 1
    uint16 characterIndex;
    /// @notice For generative fonts with randomness (Zalgo), we generate and fix this on minting. For some emojis, it can be set by the user to influence the skin color
    uint8 characterModifier;
}

There can be at most 7 tiles, which means we could store all 7 tiles inside just one uint256:

• tile 0: bits 0 -> 31
• tile 1: bits 32 -> 63 ...
• tile 6: bits 192 -> 227

This would serve as a major gas optimization, since we would only read 1 uint256 slot from storage for each tray, instead of 7 uint256 slots. These 7 reads are done extensively across the Namespace protocol implementation, since they are part of Tray.mint, Tray.buy, Tray.tokenURI, as well as Namespace.fuse, basically the functions that will be called 99.5% of the time.

Recommendation:

Will write below how the code would look for getTile.

mapping(uint256 => TileData[TILES_PER_TRAY]) private tiles;

function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData) {
    if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
    tileData = tiles[_trayId][_tileOffset];
}
-> 
uint256 private constant BITS_PER_TILE = 32;
uint256 private constant 32BIT_BITMASK = (1 << BITS_PER_TILE) - 1;
uint256 private constant 16BIT_BITMASK = (1 << 16) - 1;
uint256 private constant 8BIT_BITMASK = (1 << 8) - 1;

mapping(uint256 => uint256) private tiles;

function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData) {
    if (!_exists(_trayId)) revert TrayNotMinted(_trayId);

    uint256 tileAtOffset = (tiles[_trayId] >> (_tileOffset * BITS_PER_TILE)) & 32BIT_BITMASK;
    tileData.fontClass = tileAtOffset & 8BIT_BITMASK;
    tileAtOffset = tileAtOffset >> 8;

    tileData.characterIndex = tileAtOffset & 16BIT_BITMASK;
    tileAtOffset = tileAtOffset >> 16;

    tileData.characterModifier = tileAtOffset & 8BIT_BITMASK;
}

[G-02] ProfilePicture.getPFP could be reordered for cleaner code, and to save gas

Recommendation:

canto-pfp-protocol/src/ProfilePicture.sol:95-103
    nftContract = pictureData.nftContract;
    nftID = pictureData.nftID;
    uint256 cidNFTID = cidNFT.getPrimaryCIDNFT(subprotocolName, _pfpID);
    IAddressRegistry addressRegistry = cidNFT.addressRegistry();
    if (cidNFTID == 0 || addressRegistry.getAddress(cidNFTID) != ERC721(nftContract).ownerOf(nftID)) {
        nftContract = address(0);
        nftID = 0; // Strictly not needed because nftContract has to be always checked, but reset nevertheless to 0
    }
    -> 
    uint256 cidNFTID = cidNFT.getPrimaryCIDNFT(subprotocolName, _pfpID);
    IAddressRegistry addressRegistry = cidNFT.addressRegistry();
    if (cidNFTID > 0 || addressRegistry.getAddress(cidNFTID) == ERC721(nftContract).ownerOf(pictureDate.nftID)) {
        nftContract = pictureData.nftContract;
        nftID = pictureData.nftID;
    }

[G-03] Can use storage references to save gas

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L45

Recommendation:

canto-bio-protocol/src/Bio.sol:45
    string memory bioText = bio[_id];
    -> 
    string storage bioText = bio[_id];

[G-04] For font classes 2, 3, 4, 5, 8, 9, startingSequence contains 4 bytes but only the last 2 are ever used

When determining the startingSequence, all classes have the first two bytes the same: "F09D", hence they are redundant, which is also shown in the _getUtfSequence function, which only uses bytes 3 and 4 (positions 2 and 3).

Recommendation:

Only return the last 2 bytes for each startingSequence. Update the _getUtfSequence function to work with positions 0 and 1 instead, and return "F09D" + _startingSequence instead.

[G-05] Use calldata instead of memory for function arguments that do not get mutated

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

Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.

[G-06] Check that tray with _trayId exists can be extracted to modifier to save gas and improve readability

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

Recommendation:

modifier trayMinted(uint256 _trayId) {
    if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
    _;
}

function getTiles(uint256 _trayId) external view trayMinted(_trayId) returns (TileData[TILES_PER_TRAY] memory tileData) {
    if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
    tileData = tiles[_trayId];
}

[G-07] iteratePRNG could be simpler to save gas, while giving the same amount of pseudo-randomness

Unless there is a mathematical explanation about the iteratePRNG algorithm, gas could be saved by doing less multiplications, which would give the same amount of pseudorandomness.

Recommendation:

    function iteratePRNG(uint256 _currState) internal pure returns (uint256 iteratedState) {
        unchecked {
            iteratedState = (_currState ** 2) % 2038074743;
        }
    }

[G-08] State variables that don't change should be marked as immutable to save gas

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

[G-09] Use assembly to write address storage values

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

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

Recommendation:

canto-pfp-protocol/ProfilePicture/src/ProfilePicture.sol:84:
    pictureData.nftContract = _nftContract;
    ->
    assembly {
        sstore(pictureData.nftContract.slot, _nftContract)
    }

[G-10] To prevent having to redeploy contracts and waste gas, require immutable storage members to have been provided values in constructors

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L78 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L50 https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L37

Recommendation

lastHash = _initHash;
trayPrice = _trayPrice;
revenueAddress = _revenueAddress;
note = ERC20(_note);
namespaceNFT = _namespaceNFT;
->
if (_trayPrice == 0) revert TrayPriceCantBeZero;
if (_namespaceNFT == address(0)) revert NamespaceNFTCantBeZeroAddress;
lastHash = _initHash;
trayPrice = _trayPrice;
revenueAddress = _revenueAddress;
note = ERC20(_note);
namespaceNFT = _namespaceNFT;

[G-11] Functions guaranteed to revert if called by users can be marked payable to save gas

When functions are not payable, Solidity compiler introduces 10 more opcodes, to assert that msg.value is 0 => CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2), which cost about 21 gas per call to the function, in addition to extra deployment cost.

onlyOwner functions could be payable, as owners are less prone to send ETH to the contract by mistake.

All constructors can also be made payable, to avoid additional gas cost on deployment.

The above is a saving with no security risk, other than owners accidentally sending ether to the smart contracts.

[G-12] Forgotten delete in Namespace.burn would recover gas

Burning a Namespace NFT deletes the tokenToName and nameToToken mapping entries, but doesn't delete nftCharacters[_id], which holds the TileData information for that specific Namespace NFT.

Recommendation:

delete tokenToName[_id];
delete nameToToken[associatedName];
->
delete nftCharacters[_id];
delete tokenToName[_id];
delete nameToToken[associatedName];

[G-13] Whole Namespace.fuse function can be re-written to prioritise revert checks

Functions that revert should prioritise the revert code first and then the effects. Reverting as early as possible saves the calling user as much gas as possible, while adding no security risks or complexity.

Recommendation:

Rewrite Namespace.fuse like so:

function fuse(CharacterData[] calldata _characterList) external {
    uint256 numCharacters = _characterList.length;
    if (numCharacters > 13 || numCharacters == 0) revert InvalidNumberOfCharacters(numCharacters);

    uint256 fusingCosts = 2**(13 - numCharacters) * 1e18;
    SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, fusingCosts);

    uint256 namespaceIDToMint = ++nextNamespaceIDToMint;
    Tray.TileData[] storage nftToMintCharacters = nftCharacters[namespaceIDToMint];

    bytes memory bName = new bytes(numCharacters * 33); // Used to convert into a string. Can be 33 times longer than the string at most (longest zalgo characters is 33 bytes)
    uint256 numBytes;

    // Extract unique trays for burning them later on
    uint256 numUniqueTrays;
    uint256[] memory uniqueTrays = new uint256[](_characterList.length);

    for (uint256 i; i < numCharacters; ++i) {
        bool isLastTrayEntry = true;
        uint256 trayID = _characterList[i].trayID;
        uint8 tileOffset = _characterList[i].tileOffset;
        // Check for duplicate characters in the provided list. 1/2 * n^2 loop iterations, but n is bounded to 13 and we do not perform any storage operations
        for (uint256 j = i + 1; j < numCharacters; ++j) {
            if (_characterList[j].trayID == trayID) {
                if (_characterList[j].tileOffset == tileOffset) revert FusingDuplicateCharactersNotAllowed();
                isLastTrayEntry = false;
            }
        }

        if (isLastTrayEntry) {
            // Verify address is allowed to fuse
            address trayOwner = tray.ownerOf(trayID);
            if (
                trayOwner != msg.sender &&
                tray.getApproved(trayID) != msg.sender &&
                !tray.isApprovedForAll(trayOwner, msg.sender)
            ) revert CallerNotAllowedToFuse();

            uniqueTrays[numUniqueTrays++] = trayID;
        }

        Tray.TileData memory tileData = tray.getTile(trayID, tileOffset); // Will revert if tileOffset is too high
        if (tileData.fontClass != 0 && _characterList[i].skinToneModifier != 0) {
            revert CannotFuseCharacterWithSkinTone();
        }
        
        uint8 characterModifier = tileData.characterModifier;
        if (tileData.fontClass == 0) {
            // Emoji
            characterModifier = _characterList[i].skinToneModifier;
        }

        bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);
        tileData.characterModifier = characterModifier;
        uint256 numBytesChar = charAsBytes.length;
        charAsBytes = bytes.concat(charAsBytes, )
        for (uint256 j; j < numBytesChar; ++j) {
            bName[numBytes + j] = charAsBytes[j];
        }
        numBytes += numBytesChar;
        nftToMintCharacters.push(tileData);
        // We keep track of the unique trays NFTs (for burning them) and only check the owner once for the last occurence of the tray
    }
    // Set array to the real length (in bytes) to avoid zero bytes in the end when doing the string conversion
    assembly {
        mstore(bName, numBytes)
    }
    string memory nameToRegister = string(bName);
    uint256 currentRegisteredID = nameToToken[nameToRegister];
    if (currentRegisteredID != 0) revert NameAlreadyRegistered(currentRegisteredID);
    nameToToken[nameToRegister] = namespaceIDToMint;
    tokenToName[namespaceIDToMint] = nameToRegister;

    for (uint256 i; i < numUniqueTrays; ++i) {
        tray.burn(uniqueTrays[i]);
    }
    _mint(msg.sender, namespaceIDToMint);
    // Although _mint already emits an event, we additionally emit one because of the name
    emit NamespaceFused(msg.sender, namespaceIDToMint, nameToRegister);
}