Canto Identity Subprotocols contest - 0xdaydream's results

[L-01] Prefer using _safeMint over _mint

Bio.sol, Namespace.sol, Tray.sol and ProfilePicture.sol use ERC721's _mint method, which is missing the check if the account to mint the NFT to is a smart contract that can handle ERC721 tokens. The _safeMint method does exactly this, so prefer using it over _mint but always add a nonReentrant modifier, since calls to _safeMint can reenter.

File: canto-bio-protocol/src/Bio.sol
126:    _mint(msg.sender, tokenId);

Bio.sol#L126

File: canto-namespace-protocol/src/Namespace.sol
177:    _mint(msg.sender, namespaceIDToMint);

Bio.sol#L177

File: canto-namespace-protocol/src/Tray.sol
167:    _mint(msg.sender, _amount); // We do not use _safeMint on purpose here to disallow callbacks and save gas

Bio.sol#L167

File: canto-pfp-protocol/src/ProfilePicture.sol
126:    _mint(msg.sender, tokenId);

Bio.sol#L86

[N-01] Incosistent banner-like comments

STATE comment in ProfilePicture.sol is duplicated and can be removed for consistency and clarity.

/*//////////////////////////////////////////////////////////////
                                STATE
//////////////////////////////////////////////////////////////*/

/// @notice Reference to the CID NFT
ICidNFT private immutable cidNFT;
-
-    /*//////////////////////////////////////////////////////////////
-                                    STATE
-    //////////////////////////////////////////////////////////////*/

/// @notice Data that is stored per PFP
struct ProfilePictureData {
    /// @notice Reference to the NFT contract
    address nftContract;
    /// @notice Referenced nft ID
    uint256 nftID;
}

ProfilePicture.sol#L15

[N-02] Incomplete NatSpec Comments

NatSpec comments provide rich code documentation. The following functions are some examples that miss the @return comment. Please consider completing the NatSpec comments for functions like these.

File: canto-pfp-protocol/src/ProfilePicture.sol
70:    function tokenURI(uint256 _id) public view override returns (string memory) {

ProfilePicture.sol#L70

File: canto-bio-protocol/src/Bio.sol
43:    function tokenURI(uint256 _id) public view override returns (string memory) {

Bio.sol#L43

File: canto-namespace-protocol/src/Namespace.sol
90:    function tokenURI(uint256 _id) public view override returns (string memory) {

Namespace.sol#L90

File: canto-namespace-protocol/src/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) {

Tray.sol#L119

Tray.sol#L195

Tray.sol#L203

Tray.sol#L276

File: canto-namespace-protocol/src/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)

Utils.sol#L73

Utils.sol#L222

Utils.sol#L267

[N-03] Missing NatSpec comments

NatSpec comments provide rich code documentation. The following functions are some examples that miss NatSpec comments. Please consider adding NatSpec comments for functions like these.

File: canto-namespace-protocol/src/Tray.sol
231:    function _beforeTokenTransfers(
245:    function _drawing(uint256 _seed) private pure returns (TileData memory tileData) {

Tray.sol#L231

Tray.sol#L245

Summary

Details

[G-01] x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables

Using the addition operator instead of plus-equals saves 113 gas.

1 issue - 1 file:

-    numBytes += numBytesChar;
+    numBytes = numBytes + numBytesChar;

Namespace.sol#L150

[G-02] Setting the constructor to payable

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 15 gas on deployment with no security risks.

4 issues - 4 files:

-    constructor(address _cidNFT, string memory _subprotocolName) ERC721("Profile Picture", "PFP") {
+    constructor(address _cidNFT, string memory _subprotocolName) payable ERC721("Profile Picture", "PFP") {

ProfilePicture.sol#L57

    constructor(
        bytes32 _initHash,
        uint256 _trayPrice,
        address _revenueAddress,
        address _note,
        address _namespaceNFT
-    ) ERC721A("Namespace Tray", "NSTRAY") Owned(msg.sender) {
+    ) payable ERC721A("Namespace Tray", "NSTRAY") Owned(msg.sender) {

Tray.sol#L104

    constructor(
        address _tray,
        address _note,
        address _revenueAddress
-    ) ERC721("Namespace", "NS") Owned(msg.sender) {
+    ) payable ERC721("Namespace", "NS") Owned(msg.sender) {

Namespace.sol#L77

-    constructor() ERC721("Biography", "Bio") {
+    constructor() payable ERC721("Biography", "Bio") {

Bio.sol#L32

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

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. The extra opcodes avoided costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

5 issues - 2 files:

-    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
+    function changeNoteAddress(address _newNoteAddress) external payable onlyOwner {

Namespace.sol#L196

-    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
+    function changeRevenueAddress(address _newRevenueAddress) external payable onlyOwner {

Namespace.sol#L204

-    function changeNoteAddress(address _newNoteAddress) external onlyOwner {
+    function changeNoteAddress(address _newNoteAddress) external payable onlyOwner {

Tray.sol#L210

-    function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
+    function changeRevenueAddress(address _newRevenueAddress) external payable onlyOwner {

Tray.sol#L218

-    function endPrelaunchPhase() external onlyOwner {
+    function endPrelaunchPhase() external payable onlyOwner {

Tray.sol#L225

[G-04] Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.

See more here, here and here.

For example, Tray.sol function names can be named and sorted according to METHOD ID:

Sighash   |   Function Signature
========================
c87b56dd  =>  tokenURI(uint256)
d96a094a  =>  buy(uint256)
42966c68  =>  burn(uint256)
35e37c72  =>  getTile(uint256,uint8)
399d0f4c  =>  getTiles(uint256)
21496de6  =>  changeNoteAddress(address)
92f8b9a3  =>  changeRevenueAddress(address)
ac4081d4  =>  endPrelaunchPhase()
ef435773  =>  _beforeTokenTransfers(address,address,uint256,uint256)
a66a9122  =>  _drawing(uint256)
98995f77  =>  _startTokenId()

[G-05] Use double if statements instead of &&

Using nested if is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

4 issues - 2 files:

-    if (tileData.fontClass != 0 && _characterList[i].skinToneModifier != 0) {
-        revert CannotFuseCharacterWithSkinTone();
-    }
+    if (tileData.fontClass != 0) {
+        if (_characterList[i].skinToneModifier != 0) {
+            revert CannotFuseCharacterWithSkinTone();
+        }
+    }

Namespace.sol#L136

-    if (nftOwner != msg.sender && getApproved[_id] != msg.sender && !isApprovedForAll[nftOwner][msg.sender])
-        revert CallerNotAllowedToBurn();
+    if (nftOwner != msg.sender) {
+        if (getApproved[_id] != msg.sender) {
+            if (!isApprovedForAll[nftOwner][msg.sender]) {
+                revert CallerNotAllowedToBurn();
+            }
+        }
+    }

Namespace.sol#L186

-    if (numPrelaunchMinted != type(uint256).max && _id <= numPrelaunchMinted)
-        revert PrelaunchTrayCannotBeUsedAfterPrelaunch(_id);
+    if (numPrelaunchMinted != type(uint256).max) {
+        if (_id <= numPrelaunchMinted) {
+            revert PrelaunchTrayCannotBeUsedAfterPrelaunch(_id);
+        }
+    }

Tray.sol#L184

-    if (startTokenId <= numPrelaunchMinted && to != address(0))
-        revert PrelaunchTrayCannotBeUsedAfterPrelaunch(startTokenId);
+    if (startTokenId <= numPrelaunchMinted) {
+        if (to != address(0)) {
+            revert PrelaunchTrayCannotBeUsedAfterPrelaunch(startTokenId);
+        }
+    }

Tray.sol#L240

[G-06] Using private rather than public for constants saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

3 issues - 2 files:

-    Tray public immutable tray;
+    Tray private immutable tray;

Namespace.sol#L17

-    uint256 public immutable trayPrice;
+    uint256 private immutable trayPrice;

Tray.sol#L37

-    address public immutable namespaceNFT;
+    address private immutable namespaceNFT;

Tray.sol#L50

[G-07] Use a more recent version of Solidity

Solidity 0.8.10 has a useful change that reduced gas costs of external calls which expect a return value.

In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.

In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.

I recommend that you upgrade the versions of all contracts in scope to the latest version of robustness, 0.8.19.