Canto Identity Subprotocols contest - descharre's results

Lines of code

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

Vulnerability details

Impact

The tokenName and image of Namespace should always be the same. The image is correct but the tokenName only consists of emojis.

Duplicate names are checked on the tokenName and not on the image. Because of this a tile of font class 1 with character index 12 will have the same name as a tile of class 2 with index 12. Both names will be an emoji with index 12. The second NameSpace won't be created because the name is already registered even though the correct image is different.

Proof of Concept

This is because the conversion characterToUnicodeBytes has a wrong input parameter. It always has 0 as fontClass so the function characterToUnicodeBytes() in Utils.sol sees this as an emoji.

NameSpace.sol L144:
    bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);

When you log the metadata in tokenURI():

• the name is ✅🤔😪😂🎂😮☕
• the image/svg consist of the following tiles: b 9 😪 𝓇 8 😮 𝔢

You can see the emojis are correct but the other ones are all converted to an emoji

Tools Used

Foundry

Recommended Mitigation Steps

This can easily be fixed by changing the input parameter to tileData.fontClass.

NameSpace.sol L144:
-    bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);
+    bytes memory charAsBytes = Utils.characterToUnicodeBytes(tileData.fontClass, tileData.characterIndex, characterModifier);

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] Missing 0 address checks in constructors

Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

• ProfilePicture.sol#L58-L59
• Tray.sol#L109
• Namespace.sol#L78

[L-02] Buying over 180 trays exceeds block gas limit

buy() is a very expensive function. Around 180 trays can be bought at once before exceeding the block gas limit of 30 million. This also means the owner can't mint all 1000 of them during prelaunch in one transaction. One mitigation is to have a limit on _amount and add a require statement.

[L-03] Transaction shouldn't revert when emoji does not support skin modifier

When an emoji does not support a skintonemodifier but one is given, the transaction will revert. There is no reason to revert, it's better to just return the emoji with skintonemodifier 0 rather than reverting.

Utils.sol#L112-L128

        if (_characterModifier != 0) {
-           if (!supportsSkinToneModifier) revert EmojiDoesNotSupportSkinToneModifier(_characterIndex);
+           if (!supportsSkinToneModifier) return character;
            if (_characterModifier == 1) {
                character = abi.encodePacked(character, hex"F09F8FBB");
            } else if (_characterModifier == 2) {
                character = abi.encodePacked(character, hex"F09F8FBC");
            } else if (_characterModifier == 3) {
                character = abi.encodePacked(character, hex"F09F8FBD");
            } else if (_characterModifier == 4) {
                character = abi.encodePacked(character, hex"F09F8FBE");
            } else if (_characterModifier == 5) {
                character = abi.encodePacked(character, hex"F09F8FBF");
            } else {
                revert InvalidSkinToneModifierProvided(_characterModifier);
            }
        }
        return character;

[L-04] TokenURI of ProfilePicture will revert when it's not connected to the CIDNFT

When a user mints a ProfilePicture but doesn't immediately add it to his CIDNFT, the function tokenURI() will revert with the errorCode PFPNoLongerOwnedByOriginalOwner. Even though the owner probably still has the NFT. This is because the function getPFP() will have a cidNFTID of 0. This might lead to users reminting their NFT because they think they have the wrong picture.

A better way do this would be to update a mapping everytime the pfp nft gets minted and check if that matches the owner of the nft.

L34:
+    mapping(uint256 => address) private pfpMinter;

L79:
    function mint(address _nftContract, uint256 _nftID) external {
        uint256 tokenId = ++numMinted;
        if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
            revert PFPNotOwnedByCaller(msg.sender, _nftContract, _nftID);
        ProfilePictureData storage pictureData = pfp[tokenId];
        pictureData.nftContract = _nftContract;
        pictureData.nftID = _nftID;
+       pfpMinter[tokenId] = msg.sender;
        _mint(msg.sender, tokenId);
        emit PfpAdded(msg.sender, tokenId, _nftContract, _nftID);
    }

L94:
    function getPFP(uint256 _pfpID) public view returns (address nftContract, uint256 nftID) {
        if (_ownerOf[_pfpID] == address(0)) revert TokenNotMinted(_pfpID);
        ProfilePictureData storage pictureData = pfp[_pfpID];
        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)) {
+       if (pfpMinter[_pfpID] != ERC721(nftContract).ownerOf(nftID)) {
            nftContract = address(0);
            nftID = 0; // Strictly not needed because nftContract has to be always checked, but reset nevertheless to 0
        }
    }

Non critical

[N-01] Unspecific compiler version pragma

Avoid floating pragmas for non-library contracts. Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

[N-02] Attach SafeTransferLib to ERC20

You can attach library function to any type with the for statement. This is used in most protocols and is the most readable.

Tray.sol#L157

L14:
+   using SafeTransferLib for ERC20;

L157:
-   SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, _amount * trayPrice);
+   note.safeTransferFrom(msg.sender, revenueAddress, _amount * trayPrice);

The same can be done in Namespace.

[N-03] Unnecesarry variable assignments

It's useless to write a memory variable to another memory variable. Even when it's a struct, it doesn't save any more gas. The extra assignments can be removed

Namespace.sol#L133-L145

        Tray.TileData memory tileData = tray.getTile(trayID, tileOffset); // Will revert if tileOffset is too high
-       uint8 characterModifier = tileData.characterModifier;

        if (tileData.fontClass != 0 && _characterList[i].skinToneModifier != 0) {
            revert CannotFuseCharacterWithSkinTone();
        }
            
        if (tileData.fontClass == 0) {
            // Emoji
-           characterModifier = _characterList[i].skinToneModifier;
+           tileData.characterModifier = _characterList[i].skinToneModifier;
        }
-       bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);
+       bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, tileData.characterModifier);
-       tileData.characterModifier = characterModifier;

[N-04] mapping nftCharacters does not get deleted

When you burn() a namespace, the mappings tokenToName and [nameToToken] gets deleted. But the mapping nftCharacters is also associated to that nameSpace so it should also be deleted.

[N-05] Be consistent with how you generate metadata

The function tokenURI() in Namespace and Tray both use abi.encodePacked to generate the json metadata. The function tokenURI() in Bio uses string.concat. It's best to make this consistent accross the whole protocol.

Summary

Details

[G-01] Require or revert statements that check input arguments should be at the top of the function

In this case no gas is wasted when a function is destined to fail.

• ProfilePicture.sol#L81-L82

[G-02] Burn all prelaunch trays and remove every check

The functions tokenURI() burn() and when you mint/transfer a token _beforeTokenTransfers() all check whether the tray is a prelaunch tray or not. The prelaunch trays are useless after the prelaunch period so it's better to burn them all so you don't have to perform all those checks.

Burning the trays and removing the checks will save gas for every mint, burn and transfer of a tray.

_beforeTokenTransfers() can then just be empty.

burn()

    function burn(uint256 _id) external {
        address trayOwner = ownerOf(_id);
        if (
            namespaceNFT != msg.sender &&
            trayOwner != msg.sender &&
            getApproved(_id) != msg.sender &&
            !isApprovedForAll(trayOwner, msg.sender)
        ) revert CallerNotAllowedToBurn();
-        if (msg.sender == namespaceNFT) {
-            // Disallow fusing for prelaunch trays after phase has ended
-            uint256 numPrelaunchMinted = prelaunchMinted;
-            if (numPrelaunchMinted != type(uint256).max && _id <= numPrelaunchMinted)
-                revert PrelaunchTrayCannotBeUsedAfterPrelaunch(_id);
-        }
        delete tiles[_id];
        _burn(_id);
    }

Removing the check on tokenURI() doesn't save any gas because it's a view function but removes a lot of code then for better readability.

[G-03] Don't create a seperate loop for burning trays

When you fuse trays, the function fuse() holds an array of unique trays so it can burn them at the end of the function. This is not necessary and the trays can be instantly burned instead of adding them to the uniqueTrays array. This is not a problem because if function revert all the changes to the state will also revert.

2269 gas can be saved with the following optimization:

L120:
-   uint256 numUniqueTrays;
-   uint256[] memory uniqueTrays = new uint256[](_characterList.length);

L153:
    if (isLastTrayEntry) {
-       uniqueTrays[numUniqueTrays++] = trayID;
        // 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();
+       tray.burn(trayID);
    }

L174:
-   for (uint256 i; i < numUniqueTrays; ++i) {
-       tray.burn(uniqueTrays[i]);
-   }

[G-04] Add unchecked block for _drawing()

The function _drawing() does a lot of calculations. There is no risk of over/underflow because there is no input parameter that the user can choose. The default "checked" behavior costs more gas when calculating, because under-the-hood the EVM does extra checks for over/underflow. Adding unchecked for the whole _drawing() function saves around 150 gas.