Canto Identity Subprotocols contest - slvDev's results

[L-01] A single-step ownership transfer pattern is dangerous

Inheriting from solmate Owned contract means you are using a single-step ownership transfer pattern. If an admin provides an incorrect address for the new owner this will result in none of the onlyOwner marked methods being callable again. The better way to do this is to use a two-step ownership transfer approach, where the new owner should first claim his new rights before they are transferred. Use OpenZeppelin's Ownable2Step instead of solmate Owned

[L-02] Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows when casting from uint256

File: canto-bio-protocol\src\Bio.sol

77:		uint8(bioTextBytes[i + 4]) >= 187 &&
78:		uint8(bioTextBytes[i + 4]) <= 191) ||

File: canto-namespace-protocol\src\Tray.sol

250:   tileData.characterIndex = uint16(charRandValue % NUM_CHARS_EMOJIS);
252:   tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS);
255:   tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS_NUMBERS);
259:   tileData.fontClass = 3 + uint8((res - 80) / 8);
261:   tileData.fontClass = 5 + uint8((res - 96) / 4);
263:   tileData.fontClass = 7 + uint8((res - 104) / 2);
267:   tileData.characterModifier = uint8(zalgoSeed % 256);

File: canto-namespace-protocol\src\Utils.sol

135:   return abi.encodePacked(bytes1(asciiStartingIndex + uint8(_characterIndex)));
145:   bytes memory character = abi.encodePacked(bytes1(asciiStartingIndex + uint8(_characterIndex)));
215:   return _getUtfSequence(startingSequence, uint8(_characterIndex));
272:   uint8 lastByteVal = uint8(_startingSequence[3]);
278:   _startingSequence[2] = bytes1(uint8(_startingSequence[2]) + 1);

[L-03] Solmate Owned Lacks Address(0) Check in transferOwnership() Function

Recomindated to use OpenZeppelin's Ownable instead of Solmate Owned.

File: \lib\solmate\src\auth\Owned.sol

function transferOwnership(address newOwner) public virtual onlyOwner {
    owner = newOwner;
    emit OwnershipTransferred(msg.sender, newOwner);
}

[L-04] Prefer using _safeMint over _mint

Tray.sol contract use _mint with an explanation of why, but other contracts without explanation 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

File: canto-bio-protocol\src\Bio.sol

function mint(string calldata _bio) external {
    ...
    _mint(msg.sender, tokenId);
		...
}

File: canto-namespace-protocol\src\Namespace.sol

function fuse(CharacterData[] calldata _characterList) external {
    ...
    _mint(msg.sender, namespaceIDToMint);
    ...
}

File: canto-pfp-protocol\src\ProfilePicture.sol

function mint(address _nftContract, uint256 _nftID) external {
  ...
  _mint(msg.sender, tokenId);
}

[L-05] Missing Event for mint and burn

Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

File: canto-namespace-protocol\src\Namespace.sol

function burn(uint256 _id) external {
    address nftOwner = ownerOf(_id);
    if (nftOwner != msg.sender && getApproved[_id] != msg.sender && !isApprovedForAll[nftOwner][msg.sender])
        revert CallerNotAllowedToBurn();
    string memory associatedName = tokenToName[_id];
    delete tokenToName[_id];
    delete nameToToken[associatedName];
    _burn(_id);
}

File: canto-namespace-protocol\src\Tray.sol

function buy(uint256 _amount) external {
	   ...
    _mint(msg.sender, _amount);

function burn(uint256 _id) external {
    ...
    _burn(_id);
}

[L-06] A single point of failure

The onlyOwner role has a single point of failure and onlyOwner can use critical a few functions.

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.

onlyOwner functions:

File: canto-namespace-protocol\src\Namespace.sol

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {

File: canto-namespace-protocol\src\Tray.sol

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
function endPrelaunchPhase() external onlyOwner {

[L-07] Solmate’s SafeTransferLib doesn’t check whether the ERC20 contract exists

Solmate’s SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn’t exist (yet).

File: canto-namespace-protocol\src\Namespace.sol

function fuse(CharacterData[] calldata _characterList) external {
    ...
    SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, fusingCosts);
    ...
}

File: canto-namespace-protocol\src\Tray.sol

function buy(uint256 _amount) external {
   ...
   SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, _amount * trayPrice);
   ...
}

[Q-01] Consider making subprotocolName constant

Since there is no functionality to change subprotocolName and subprotocol contract deployed only once consider making subprotocolName variable constant.

File: canto-pfp-protocol\src\ProfilePicture.sol

string public subprotocolName;

[Q-02] Miss checks for immutable variables in the constructor

Since these variables can't be changed, good practice to add validation(range) and address(0) checks.

File: canto-pfp-protocol\src\ProfilePicture.sol

ICidNFT private immutable cidNFT;
constructor(address _cidNFT, string memory _subprotocolName) ERC721("Profile Picture", "PFP") {
    cidNFT = ICidNFT(_cidNFT);
    ...
}

File: canto-namespace-protocol\src\Namespace.sol

Tray public immutable tray;
constructor(address _tray,address _note,address _revenueAddress) ERC721("Namespace", "NS") Owned(msg.sender) {
    tray = Tray(_tray);
    ...
}

File: canto-namespace-protocol\src\Tray.sol

uint256 public immutable trayPrice;
address public immutable namespaceNFT;

constructor(bytes32 _initHash,uint256 _trayPrice,address _revenueAddress,address _note,address _namespaceNFT) ERC721A("Namespace Tray", "NSTRAY") Owned(msg.sender) {
    ...
    trayPrice = _trayPrice;
    ...
    namespaceNFT = _namespaceNFT;
    ...
}

[Q-03] Missing checks for address(0) when assigning values to address state variables

These cases are missed by the automatic finding script

File: canto-namespace-protocol\src\Namespace.sol

ERC20 public note;
function changeNoteAddress(address _newNoteAddress) external onlyOwner {
    address currentNoteAddress = address(note);
    note = ERC20(_newNoteAddress);
    emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
}

File: canto-namespace-protocol\src\Tray.sol

ERC20 public note;
function changeNoteAddress(address _newNoteAddress) external onlyOwner {
    address currentNoteAddress = address(note);
    note = ERC20(_newNoteAddress);
    emit NoteAddressUpdate(currentNoteAddress, _newNoteAddress);
}

[Q-04] Remove an unused function

The declared internal function is not used in any other function.

Similar functionality used tokenURI(uint256 _id) I suppose this function was used or should be used there but it didn’t.

File: canto-namespace-protocol\src\Tray.sol

function _beforeTokenTransfers(address,address to,uint256 startTokenId,uint256) internal view override {
    uint256 numPrelaunchMinted = prelaunchMinted;
    if (numPrelaunchMinted != type(uint256).max) {
        // We do not allow any transfers of the prelaunch trays after the phase has ended
        if (startTokenId <= numPrelaunchMinted && to != address(0))
            revert PrelaunchTrayCannotBeUsedAfterPrelaunch(startTokenId);
    }
}

function tokenURI(uint256 _id) public view override returns (string memory) {
    ...
    uint256 numPrelaunchMinted = prelaunchMinted;
    if (numPrelaunchMinted != type(uint256).max) {
        // Prelaunch trays become invalid after the phase has ended
        if (_id <= numPrelaunchMinted) revert PrelaunchTrayCannotBeUsedAfterPrelaunch(_id);
    }
    ...
}

[Q-05] Incomplete NatSpec Comments

Missing NatSpet for a contract in all contracts. eg:

/// @title 
/// @author 
/// @notice 
/// @dev 
/// @custom:experimental 

Missing @return in NatSpec

File: canto-pfp-protocol\src\ProfilePicture.sol

function tokenURI(uint256 _id) public view override returns (string memory) {

File: canto-namespace-protocol\src\Namespace.sol

function tokenURI(uint256 _id) public view override returns (string memory) {

File: canto-namespace-protocol\src\Tray.sol

function tokenURI(uint256 _id) public view override returns (string memory) {
function getTile(uint256 _trayId, uint8 _tileOffset) external view returns (TileData memory tileData) {
function getTiles(uint256 _trayId) external view returns (TileData[TILES_PER_TRAY] memory tileData) {

File: canto-bio-protocol\src\Bio.sol

function tokenURI(uint256 _id) public view override returns (string memory) {

[Q-06] Missing NatSpec comments

File: canto-namespace-protocol\src\Tray.sol

function _beforeTokenTransfers(address,address to, uint256 startTokenId,uint256) internal view override {
function _drawing(uint256 _seed) private pure returns (TileData memory tileData) {

[Q-07] Function writing that does not comply with the Solidity Style Guide

Order of Functions; ordering helps readers identify which functions they can call and find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private
• within a grouping, place the view and pure functions last

Files:

canto-pfp-protocol\src\ProfilePicture.sol
canto-namespace-protocol\src\Namespace.sol
canto-namespace-protocol\src\Tray.sol
canto-bio-protocol\src\Bio.sol

[Q-08] Uppercase immutable variables

File: canto-namespace-protocol\src\Namespace.sol
Tray public immutable tray;

File: canto-namespace-protocol\src\Tray.sol
uint256 public immutable trayPrice;
address public immutable namespaceNFT;

File: canto-pfp-protocol\src\ProfilePicture.sol
ICidNFT private immutable cidNFT;

[G-01] Use nested if, avoid multiple check combinations

Using nested if is cheaper than using && multiple check combinations if it ****is not followed by an else statement. There are more advantages, such as easier-to-read code and better coverage reports.

File: canto-namespace-protocol\src\Namespace.sol

if (tileData.fontClass != 0 && _characterList[i].skinToneModifier != 0) {
if (nftOwner != msg.sender && getApproved[_id] != msg.sender && !isApprovedForAll[nftOwner][msg.sender])

File: canto-namespace-protocol\src\Tray.sol

if (numPrelaunchMinted != type(uint256).max && _id <= numPrelaunchMinted)
if (startTokenId <= numPrelaunchMinted && to != address(0))

[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 gas on deployment with no security risks.

All contracts

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

File: canto-namespace-protocol\src\Namespace.sol

constructor(address _tray,address _note,address _revenueAddress) ERC721("Namespace", "NS") Owned(msg.sender) {
		...
    note = ERC20(_note);
    revenueAddress = _revenueAddress;
    ...
}

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
    ...
    note = ERC20(_newNoteAddress);
    ...
}

/// @notice Change the revenue address
/// @param _newRevenueAddress New address to use
function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
    ...
    revenueAddress = _newRevenueAddress;
    ...
}

File: canto-namespace-protocol\src\Tray.sol

constructor(bytes32 _initHash,uint256 _trayPrice,address _revenueAddress,address _note,address _namespaceNFT) ERC721A("Namespace Tray", "NSTRAY") Owned(msg.sender) {
    ...
    revenueAddress = _revenueAddress;
    note = ERC20(_note);
    ...
}

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
    ...
    note = ERC20(_newNoteAddress);
    ...
}

function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
    ...
    revenueAddress = _newRevenueAddress;
    ...
}

[G-04] Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

File: canto-namespace-protocol\src\Tray.sol

function tokenURI(uint256 _id) public view override returns (string memory) {
    ...
-    TileData[TILES_PER_TRAY] storage storedNftTiles = tiles[_id];
+    TileData[TILES_PER_TRAY] memory storedNftTiles = tiles[_id];
    ...
}

+   Overall gas change: -1371

[G-05] Use constants instead of type(uintx).max

It uses more gas in the distribution process and also for each transaction than constant usage.

File: canto-namespace-protocol\src\Tray.sol

uint256 private prelaunchMinted = type(uint256).max;
if (numPrelaunchMinted != type(uint256).max) {
if (prelaunchMinted == type(uint256).max) {
if (numPrelaunchMinted != type(uint256).max && _id <= numPrelaunchMinted)
if (prelaunchMinted != type(uint256).max) revert PrelaunchAlreadyEnded();
if (numPrelaunchMinted != type(uint256).max) {

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

If a function modifier or require such as onlyOwner-admin 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 are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

File: canto-namespace-protocol\src\Namespace.sol

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {

File: canto-namespace-protocol\src\Tray.sol

function changeNoteAddress(address _newNoteAddress) external onlyOwner {
function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {
function endPrelaunchPhase() external onlyOwner {

[G-07] 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

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

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.

[G-08] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contracts gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html

Use a larger size than downcast where needed.

[G-09] Avoid contract existence checks by using solidity version 0.8.10 or later

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

File: canto-pfp-protocol\src\ProfilePicture.sol

turnstile.register(tx.origin);
return ERC721(nftContract).tokenURI(nftID);
if (ERC721(_nftContract).ownerOf(_nftID) != msg.sender)
uint256 cidNFTID = cidNFT.getPrimaryCIDNFT(subprotocolName, _pfpID);
IAddressRegistry addressRegistry = cidNFT.addressRegistry();
if (cidNFTID == 0 || addressRegistry.getAddress(cidNFTID) != ERC721(nftContract).ownerOf(nftID)) {

File: canto-namespace-protocol\src\Namespace.sol

Tray.TileData memory tileData = tray.getTile(trayID, tileOffset); // Will revert if tileOffset is too high
address trayOwner = tray.ownerOf(trayID);
tray.getApproved(trayID) != msg.sender &&
!tray.isApprovedForAll(trayOwner, msg.sender)
tray.burn(uniqueTrays[i]);

File: canto-namespace-protocol\src\Tray.sol

turnstile.register(tx.origin);

File: canto-bio-protocol\src\Bio.sol

turnstile.register(tx.origin);