Canto Identity Subprotocols contest - Aymen0909's results

QA Report

Summary

Findings

1- burn function doesn't delete the NFT character data :

In the burn function from the Namespace contract, when the NFT is burned its correspanding name (stored in tokenToName[_id]) and tokenId (stored in nameToToken[associatedName]) are deleted but the NFT character data stored in the nftCharacters struct are not deleted and remains even after the burning operation, this can potentially impact the protocol working in the future if this data is used for a given purpose.

Risk : Low

Proof of Concept

Issue occurs in the code below :

File: Namespace.sol Line 184-192

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];
    /** @audit
        Should also delete the nftCharacters data
    */
    _burn(_id);
}

Mitigation

Delete the NFT character data when burning a given NFT.

2- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the zero value (address(0))

Risk : Low

Proof of Concept

Instances include:

File: ProfilePicture.sol Line 58

cidNFT = ICidNFT(_cidNFT);

File: Tray.sol Line 109

namespaceNFT = _namespaceNFT;

File: Namespace.sol Line 78

tray = Tray(_tray);

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

3- Duplicated input check statements should be refactored to a modifier :

Input check statements used multiple times inside a contract should be refactored to a modifier for better readability and also to save gas.

Risk : NON CRITICAL

Proof of Concept

The following check statements are repeated multiple times :

File: Tray.sol

120     if (!_exists(_id)) revert TrayNotMinted(_id);
196     if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
204     if (!_exists(_trayId)) revert TrayNotMinted(_trayId);

Because this check is done at the start of the function it can be placed and can be refactored into a modifier to save gas, it should be replaced by a isAllowed modifier as follow :

modifier isAllowed(uint256 _id){
    if (!_exists(_id)) revert TrayNotMinted(_id);
    _;
}

4- Related data should be grouped in a struct :

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

Risk : Non critical

Proof of Concept

Instances include:

File: Namespace.sol Line 46-49

46      mapping(uint256 => string) public tokenToName;
49      mapping(uint256 => Tray.TileData[]) private nftCharacters;

Mitigation

Group the related data in a struct and use one mapping :

struct TokenToNftData {
    string tokenToName
    Tray.TileData[] nftCharacters;
}

And it would be used as a state variable :

mapping(uint256 => TokenToNftData) public tokenToNftData;

5- constant should be defined rather than using magic numbers :

It is best practice to use constant variables rather than hex/numeric literal values to make the code easier to understand and maintain, but if they are used those numbers should be well docummented.

Risk : Non critical

Proof of Concept

Instances include:

File: Namespace.sol

15485863

2038074743

Mitigation

Replace the hex/numeric literals aforementioned with constants.

Gas Optimizations

Summary

Findings

1- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 2 instances of this issue in Lottery :

File: Namespace.sol

46      mapping(uint256 => string) public tokenToName;
49      mapping(uint256 => Tray.TileData[]) private nftCharacters;

These mappings could be refactored into the following struct and mapping for example :

struct TokenToNftData {
    string tokenToName
    Tray.TileData[] nftCharacters;
}
    
mapping(uint256 => TokenToNftData) public tokenToNftData;

2- Using storage instead of memory for struct/array saves gas :

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read.

The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

There is 1 instance of this issue :

File: Bio.sol Line 45

string memory bioText = bio[_id];

3- Duplicated input check statements should be refactored to a modifier (saves ~400 gas) :

The following check statements are repeated multiple times :

File: Tray.sol

120     if (!_exists(_id)) revert TrayNotMinted(_id);
196     if (!_exists(_trayId)) revert TrayNotMinted(_trayId);
204     if (!_exists(_trayId)) revert TrayNotMinted(_trayId);

Because this check is done at the start of the function it can be placed and can be refactored into a modifier to save gas, it should be replaced by a isAllowed modifier as follow :

modifier isAllowed(uint256 _id){
    if (!_exists(_id)) revert TrayNotMinted(_id);
    _;
}

4- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 12 instances of this issue:

File: Tray.sol

56      for (uint i; i < lengthInBytes; ++i)
100     for (uint i; i < lines; ++i) 

File: Namespace.sol

122     for (uint256 i; i < numCharacters; ++i)
127     for (uint256 j = i + 1; j < numCharacters; ++j)
147     for (uint256 j; j < numBytesChar; ++j)
174     for (uint256 i; i < numUniqueTrays; ++i)

File: Utils.sol

146     for (uint256 i; i < numAbove; ++i)
155     for (uint256 i; i < numMiddle; ++i)
164     for (uint256 i; i < numBelow; ++i)
225     for (uint256 i; i < _tiles.length; ++i)

File: Bio.sol

56      for (uint i; i < lengthInBytes; ++i)
100     for (uint i; i < lines; ++i)