Canto Identity Subprotocols contest - 0xnev's results

Issues Template

Non-Critical Issues

Refactor Issues

Ordinary Issues

[N-01] For mordern and more readable code, update import usages

9 results - 5 files

/ProfilePicture.sol
5: import "../interface/Turnstile.sol";
6: import "../interface/ICidNFT.sol";

/Bio.sol
7: import "../interface/Turnstile.sol";

/Namespace.sol
7: import "./Tray.sol";
8: import "./Utils.sol";
9: import "../interface/Turnstile.sol";

/Tray.sol
10: import "./Utils.sol";
11: import "../interface/Turnstile.sol";

/Utils.sol
4: import "./Tray.sol";

Description: Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it. This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better. https://betterprogramming.pub/solidity-tutorial-all-about-imports-c65110e41f3a

Recommendation: import {contract1 , contract2} from "filename.sol";

[N-02] Solidity compiler optimizations can be problematic

/foundry.toml
1: [profile.default]
2: optimizer = true
3: optimizer_runs = 1_000
4: via-ir=true

Description Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations. Exploit Scenario: A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[N-03] Constructor lacks zero-address checks

Context: 3 results - 2 files

/Namespace.sol
73:    constructor(
74:        address _tray,
75:        address _note,
76:        address _revenueAddress
77:    ) ERC721("Namespace", "NS") Owned(msg.sender) {

80:        revenueAddress = _revenueAddress;

/Tray.sol
98:    constructor(
99:        bytes32 _initHash,
100:        uint256 _trayPrice,
101:        address _revenueAddress,
102:        address _note,
103:        address _namespaceNFT
104:    ) ERC721A("Namespace Tray", "NSTRAY") Owned(msg.sender) {

107:        revenueAddress = _revenueAddress;  
109:        namespaceNFT = _namespaceNFT;

Description: Implement Zero-address checks in the constructors, to avoid the risk of setting a storage variable as zero-address at deploy time

Recomendation: Add a zero-address check for the address variables for the instances above

[N-04] Critical address changes should use 2 step procedure

Context:

4 results - 2 files

/Namespace.sol
196:    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
197:        address currentNoteAddress = address(note);
198:        note = ERC20(_newNoteAddress);
199:        emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
200:    }

204:    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
205:        address currentRevenueAddress = revenueAddress;
206:        revenueAddress = _newRevenueAddress;
207:        emit RevenueAddressUpdated(currentRevenueAddress, _newRevenueAddress);
208:    }

/Tray.sol
210:    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
211:        address currentNoteAddress = address(note);
212:        note = ERC20(_newNoteAddress);
213:        emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
214:    }

218:    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
219:        address currentRevenueAddress = revenueAddress;
220:        revenueAddress = _newRevenueAddress;
221:        emit RevenueAddressUpdated(currentRevenueAddress, _newRevenueAddress);
222:    }

Description Lack of two-step procedure for critical operations leaves them error-prone. Consider adding a two-step procedure on the critical functions.

Recommendation: Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critical functionalities to the wrong address.

[N-05] Event missing parameter

1 result - 1 file

/Tray.sol
 80:    event PrelaunchEnded();

225:    function endPrelaunchPhase() external onlyOwner {
226:        if (prelaunchMinted != type(uint256).max) revert PrelaunchAlreadyEnded();
227:        prelaunchMinted = _nextTokenId() - 1;
228:        emit PrelaunchEnded();
229:    }

Description: There is a event missing parameter. Consider adding a parameter such as prelaunchMinted for better understanding of event emitted

[N-06] Initial value check missing in change functions

/Namespace.sol
4 result - 2 files

196:    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
197:        address currentNoteAddress = address(note);
198:        note = ERC20(_newNoteAddress);
199:        emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
200:    }

204:    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
205:        address currentRevenueAddress = revenueAddress;
206:        revenueAddress = _newRevenueAddress;
207:        emit RevenueAddressUpdated(currentRevenueAddress, _newRevenueAddress);
208:    }

/Tray.sol
210:    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
211:        address currentNoteAddress = address(note);
212:        note = ERC20(_newNoteAddress);
213:        emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
214:    }

218:    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
219:        address currentRevenueAddress = revenueAddress;
220:        revenueAddress = _newRevenueAddress;
221:        emit RevenueAddressUpdated(currentRevenueAddress, _newRevenueAddress);
222:    }

Description: There is missing initial value in change functions. Checking initial values before changing can help avoid unecessary gas usage.

Reccomendation Checking whether the current value and the new value are the same should be added

[N-07] Lack of input validation for amount of trays minted in buy function

Context:

150:   function buy(uint256 _amount) external
151:        uint256 startingTrayId = _nextTokenId();
152:        if (prelaunchMinted == type(uint256).max) {
153:            // Still in prelaunch phase
154:            if (msg.sender != owner) revert OnlyOwnerCanMintPreLaunch();
155:            if (startingTrayId + _amount - 1 > PRE_LAUNCH_MINT_CAP) revert MintExceedsPreLaunchAmount(); 
156:        } else {
157:            SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, _amount * trayPrice);
158:        }

Description: In the buy function, zero value checks can be made to ensure _amount bought is not zero and prevent wastage of gas by transferring zero funds to revenueAddress

Recommendation: Add a zero value check for _amount of trays to be minted:

function buy(uint256 _amount) external {
    if (amount == 0) revert ZeroTraysBought;
}

[R-01] Use bytes.concat()/string.concat() instead of abi.encodePacked()

24 results - 4 files

/Bio.sol
43:    function tokenURI(uint256 _id) public view override returns (string memory)
116:        return string(abi.encodePacked("data:application/json;base64,", json));

/Namespace.sol
 90:    function tokenURI(uint256 _id) public view override returns (string memory) {
 91:        if (_ownerOf[_id] == address(0)) revert TokenNotMinted(_id);
 92:        string memory json = Base64.encode(
 93:            bytes(
 94:                string(
 95:                    abi.encodePacked(
 96:                        '{"name": "',
 97:                        tokenToName[_id],
 98:                        '", "image": "data:image/svg+xml;base64,',
 99:                        Base64.encode(bytes(Utils.generateSVG(nftCharacters[_id], false))),
100:                        '"}'
101:                    )
102:                )
103:            )
104:        );
105:        return string(abi.encodePacked("data:application/json;base64,", json));
106:    }

/Tray.sol
132:        string memory json = Base64.encode(
133:            bytes(
134:                string(
135:                    abi.encodePacked(
136:                        '{"name": "Tray #',
137:                        LibString.toString(_id),
138:                        '", "image": "data:image/svg+xml;base64,',
139:                        Base64.encode(bytes(Utils.generateSVG(nftTiles, true))),
140:                        '"}'
141:                    )
142:                )
143:            )
144:        );
145:        return string(abi.encodePacked("data:application/json;base64,", json));

/Utils.sol
 73:    function characterToUnicodeBytes
104:            bytes memory character = abi.encodePacked(
105:                EMOJIS[byteOffset],
106:                EMOJIS[byteOffset + 1],
107:                EMOJIS[byteOffset + 2]
108:            );

110:                character = abi.encodePacked(character, EMOJIS[byteOffset + i]);
115:                    character = abi.encodePacked(character, hex"F09F8FBB");
117:                    character = abi.encodePacked(character, hex"F09F8FBC");
119:                    character = abi.encodePacked(character, hex"F09F8FBD");
121:                    character = abi.encodePacked(character, hex"F09F8FBE");
123:                    character = abi.encodePacked(character, hex"F09F8FBF");
135:            return abi.encodePacked(bytes1(asciiStartingIndex + uint8(_characterIndex)));
145:            bytes memory character = abi.encodePacked(bytes1(asciiStartingIndex + uint8(_characterIndex)));

149:                character = abi.encodePacked(
150:                    character,
151:                    ZALGO_ABOVE_LETTER[characterIndex],
152:                    ZALGO_ABOVE_LETTER[characterIndex + 1]
153:                );

158:                character = abi.encodePacked(
159:                    character,
160:                    ZALGO_OVER_LETTER[characterIndex],
161:                    ZALGO_OVER_LETTER[characterIndex + 1]
162:                );

167:                character = abi.encodePacked(
168:                    character,
169:                    ZALGO_BELOW_LETTER[characterIndex],
170:                    ZALGO_BELOW_LETTER[characterIndex + 1]
171:                );

197:                    return abi.encodePacked(FONT_SQUIGGLE[0], FONT_SQUIGGLE[1]);
199:                    return abi.encodePacked(FONT_SQUIGGLE[2], FONT_SQUIGGLE[3], FONT_SQUIGGLE[4]);
202:                    return abi.encodePacked(FONT_SQUIGGLE[5 + offset], FONT_SQUIGGLE[6 + offset]);
204:                    return abi.encodePacked(FONT_SQUIGGLE[47]);
206:                    return abi.encodePacked(FONT_SQUIGGLE[48], FONT_SQUIGGLE[49], FONT_SQUIGGLE[50]);

Description: Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

[R-02] Use delete instead of zero assignment to clear storage variables

Context:

5 results - 3 files

/ProfilePicture.sol
94:    function getPFP(uint256 _pfpID) public view returns (address nftContract, uint256 nftID)
102:            nftContract = address(0);
103:            nftID = 0; // Strictly not needed because nftContract has to be always checked, but reset nevertheless to 0

/Bio.sol
43:    function tokenURI(uint256 _id) public view override returns (string memory)
92:                    prevByteWasContinuation = false;
93:                    bytesOffset = 0;

/Namespace.sol
110:    function fuse(CharacterData[] calldata _characterList) external
129:                    isLastTrayEntry = false;

Description: delete has the same effect of reassigning variables to its default values based on their type and even saves gas

Reccomendation: Use delete instead of zero assignment to clear variables

[R-03] Move validation statements to top of function when validating input parameters

Context:

1 result - 1 file

/ProfilePicture.sol
79:    function mint(address _nftContract, uint256 _nftID) external
81:        if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
82:            revert PFPNotOwnedByCaller(msg.sender, _nftContract, _nftID);

Description: Input parameters that require validation should be done first in functions to avoid execution of the rest of the function logic and waste gas

Recommendation: Consider shifting the input validation to the top of the function for the above instances

[R-04] Number values can be refactored to use _

Context:

6 results - 2 files

/Utils.sol
 64:    uint256 private constant EMOJIS_BYTE_OFFSET_SIX_BYTES = 1515; // 17 * 3 + 366 * 4
 65:    uint256 private constant EMOJIS_BYTE_OFFSET_SEVEN_BYTES = 1671; // 17 * 3 + 366 * 4 + 26 * 6
 66:    uint256 private constant EMOJIS_BYTE_OFFSET_EIGHT_BYTES = 1720; // 17 * 3 + 366 * 4 + 26 * 6 + 7 * 7
 67:    uint256 private constant EMOJIS_BYTE_OFFSET_FOURTEEN_BYTES = 1744; // 17 * 3 + 366 * 4 + 26 * 6 + 7 * 7 + 3 * 8

256:    function iteratePRNG(uint256 _currState) internal pure returns (uint256 iteratedState) {
257:        unchecked {
258:            iteratedState = _currState * 15485863;
259:            iteratedState = (iteratedState * iteratedState * iteratedState) % 2038074743;
260:        }
261:    }

Description: Throughout the codebase, the project have generally practiced the use of _ for large number values except for the above instance

Reccomendation: Consider using underscore for number value to improve readability

[R-05] State variables not changed after deployment can be immutable

1 result - 1 file

/ProfilePicture.sol
35:    string public subprotocolName;

Description: State variables that are not changed after deployment time (set in constructor) can be set as immutable. This also saves deployment gas.

[O-1] Missing natspec comments

2 result - 1 file

/Tray.sol
231: function _beforeTokenTransfers
245: function _drawing(uint256 _seed) private pure returns (TileData memory tileData)

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. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

Recommendation Include Natspec comments for the above instances

[O-2] Add return parameters in natspec comments

8 results - 5 files

/ProfilePicture.sol
 70: function tokenURI(uint256 _id) public view override returns (string memory)

/Bio.sol
 43: function tokenURI(uint256 _id) public view override returns (string memory)

/Namespace.sol
 90: function tokenURI(uint256 _id) public view override returns (string memory) 

/Tray.sol
119: function tokenURI(uint256 _id) public view override returns (string memory)
195: function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData)
203: function getTiles(uint256 _trayId) external view returns (TileData[TILES_PER_TRAY] memory tileData)

/Utils.sol
 73: function characterToUnicodeBytes
222: function generateSVG(Tray.TileData[] memory _tiles, bool _isTray) internal pure returns (string memory)
267: function _getUtfSequence(bytes memory _startingSequence, uint8 _characterIndex)

Description: 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. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

If Return parameters are declared, you must prefix them with ”/// @return”.

Some code analysis programs do analysis by reading NatSpec details, if they can’t see the “@return” tag, they do incomplete analysis.

Recommendation Include return parameters in NatSpec comments

[O-3] Commented out code

Context:

1 result - 1 file

/Utils.sol
55: // uint256 constant EMOJIS_LE_FOURTEEN_BYTES = 420;

Recommendation: Remove commented out code

[O-4] Use of unlocked pragma using >=

5 results - 5 files

/ProfilePicture.sol
2: pragma solidity >=0.8.0;

/Bio.sol
2: pragma solidity >=0.8.0;

/Namespace.sol
2: pragma solidity >=0.8.0;

/Tray.sol
2: pragma solidity >=0.8.0;

/Utils.sol
2: pragma solidity >=0.8.0;

Description: All the contracts in scope uses >= to specify solidity compiler version.

Using >= without also specifying <= will lead to failures to compile, or external project incompatability, when the major version changes and there are breaking-changes, so locking of pragma is preferred regardless of the instance counts unless a contract is intended for consumption by other developers

[O-5] Contracts does not comply with order of function for solidity style guide

Functions should be laid out in the following order for ease of search

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private

Within a grouping, place the view and pure functions last.

7 results - 4 files

/ProfilePicture.sol
79: function mint(address _nftContract, uint256 _nftID) external

/Bio.sol
43: function tokenURI(uint256 _id) public view override returns (string memory)

/Namespace.sol
90: function tokenURI(uint256 _id) public view override returns (string memory)

/Tray.sol
119: function tokenURI(uint256 _id) public view override returns (string memory)
195: function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData)
203: function getTiles(uint256 _trayId) external view returns (TileData[TILES_PER_TRAY] memory tileData)
276: function _startTokenId() internal pure override returns (uint256)

[R] Convert repeated checks to a single helper function

[G-01] Initialize stack variables outside loop

9 results - 2 files

/Bio.sol
43:    function tokenURI(uint256 _id) public view override returns (string memory)
56:        for (uint i; i < lengthInBytes; ++i) {
57:            bytes1 character = bioTextBytes[i];
61:                bytes1 nextCharacter;

/Namespace.sol
110:    function fuse(CharacterData[] calldata _characterList) external
122:        for (uint256 i; i < numCharacters; ++i) {
123:            bool isLastTrayEntry = true;
124:            uint256 trayID = _characterList[i].trayID;
125:            uint8 tileOffset = _characterList[i].tileOffset;
134:            uint8 characterModifier = tileData.characterModifier;
144:            bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);
146:            uint256 numBytesChar = charAsBytes.length;
156:                address trayOwner = tray.ownerOf(trayID);

Description: Consider initializing the stack variables before the loop to avoid reinitialization on every loop

Recommendation: Consider initializing the stack variables before the loop to save gas

[G-02] Use nested if statements to avoid multiple check combinations using &&

6 result - 2 files

/Namespace.sol
110:    function fuse(CharacterData[] calldata _characterList) external 
136:            if (tileData.fontClass != 0 && _characterList[i].skinToneModifier != 0)

157:                if (
158:                    trayOwner != msg.sender &&
159:                    tray.getApproved(trayID) != msg.sender &&
160:                    !tray.isApprovedForAll(trayOwner, msg.sender)

184:    function burn(uint256 _id) external 
186:        if (nftOwner != msg.sender && getApproved[_id] != msg.sender && !isApprovedForAll[nftOwner][msg.sender])


/Tray.sol
173:    function burn(uint256 _id) external
175:        if (
176:            namespaceNFT != msg.sender &&
177:            trayOwner != msg.sender &&
178:            getApproved(_id) != msg.sender &&
179:            !isApprovedForAll(trayOwner, msg.sender)

184:            if (numPrelaunchMinted != type(uint256).max && _id <= numPrelaunchMinted)

231:    function _beforeTokenTransfers
240:            if (startTokenId <= numPrelaunchMinted && to != address(0))

Description: Using nested if statements is cheaper than using && for multiple check combinations. Additionally, it improves readability of code and better coverage reports.

Recommendation: Replace && used within if and else if statements with nested if statements

[G-03] Move validation statements to top of function when validating input parameters

Context:

1 result - 1 file

/ProfilePicture.sol
79:    function mint(address _nftContract, uint256 _nftID) external
81:        if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
82:            revert PFPNotOwnedByCaller(msg.sender, _nftContract, _nftID);

Description: Input parameters that require validation should be done first in functions to avoid execution of the rest of the function logic and waste gas

Recommendation: Consider shifting the input validation to the top of the function for the above instances

[G-04] State variables not changed after deployment can be immutable

1 result - 1 file

/ProfilePicture.sol
35:    string public subprotocolName;

Description: State variables that are not changed after deployment time (set in constructor) can be set as immutable. This avoids a Gsset (20000 gas) and saves deployment gas.

[G-05] Sort solidity operators based on short-circuiting

Context:

2 result - 2 files 

/Bio.sol
 60: if ((i > 0 && (i + 1) % 40 == 0) || prevByteWasContinuation || i == lengthInBytes - 1)

123: (bytes(_bio).length == 0 || bytes(_bio).length > 200) revert InvalidBioLength(bytes(_bio).length)

We can sequence OR/AND (|| , &) oeprators in solidity as it applies the common short circuiting rule.

This means that in the expression f(x) || g(y), if f(x) evaluates to true, g(y) will not be evaluated even if it may have side-effects.

Similarly, in the expression f(x) && g(y), if f(x) evaluates to false, g(y) will not be evaluated even if it may have side-effects.

So setting less costly function to “f(x)” and setting costly function to “g(x)” is more gas efficient.

[G-06] Use a more recent version of solidity

Use a Solidity version of at least 0.8.2 to get simple compiler automatic inlining.

Use a Solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads.

Use a Solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.\ and et bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

Use a Solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

[G-07] Functions guranteed to revert when called by normal users can be marked payable

5 results - 2 files

/Namespace.sol
196: function changeNoteAddress(address _newNoteAddress) external onlyOwner
204: function changeRevenueAddress(address _newRevenueAddress) external onlyOwner

/Tray.sol
210: function changeNoteAddress(address _newNoteAddress) external onlyOwner
218: function changeRevenueAddress(address _newRevenueAddress) external onlyOwner
225: function endPrelaunchPhase() external onlyOwner

Description: If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.

Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Recommendation: Consider marking the functions as payable. However, it is valid if protocol decides against it due to confusion, readability and possibility of receiving funds.

[G-08] Mark state variables as private instead of public

8 results - 4 files

/ProfilePicture.sol
35: string public subprotocolName;

/Bio.sol
18: mapping(uint256 => string) public bio;

/Namespace.sol
17: Tray public immutable tray;
40: uint256 public nextNamespaceIDToMint;
43: mapping(string => uint256) public nameToToken;
46: mapping(uint256 => string) public tokenToName;

/Tray.sol
50: address public immutable namespaceNFT;
70: bytes32 public lastHash;

Description: For state variables not accessed externally, we can set them as private instead of public to save significant gas

[G-09] Check amount before execution of functions for possible gas savings

Context:

1 result - 1 file

/Tray.sol
150:   function buy(uint256 _amount) external
151:        uint256 startingTrayId = _nextTokenId();
152:        if (prelaunchMinted == type(uint256).max) {
153:            // Still in prelaunch phase
154:            if (msg.sender != owner) revert OnlyOwnerCanMintPreLaunch();
155:            if (startingTrayId + _amount - 1 > PRE_LAUNCH_MINT_CAP) revert MintExceedsPreLaunchAmount(); 
156:        } else {
157:            SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, _amount * trayPrice);
158:        }

Description: Before execution of safeTransferFrom function, we should check if amount is zero to prevent wastage of gas when this function do not do anything upon execution