Shinkai Protocol - Shinkai

The namespace mapping in the ShinkaiRegistry contract holds a string that is concatenated to the user's identity when they claim a new identity. However, the mapping lacks a visibility modifier, which defaults its visibility to internal. This oversight makes it difficult for users to check the available namespaces before selecting where to concatenate their identity. As a result, users might unintentionally select the wrong namespace, leading to potential identity mismanagement.

    mapping(uint256 => string) namespaces;

Consider adding a public visibility to the namespace mapping. 

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

The ShinkaiRegistry contracts is designed to be used with a UUPS proxy pattern. However, it lacks storage gaps. Storage gaps are essential for ensuring that new state variables can be added in future upgrades without affecting the storage layout of inheriting child contracts. Without it, any addition of new state variables in future contract versions can lead to storage collisions.

Consider adding a storage gap as the last storage variable. Place the uint256[50] private __gap; variable at the end of storage layout of ShinkaiRegistry contract.

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

The ownership of the contracts can be lost as the ShinkaiRegistry and ShinkaiControlled contracts inherited from the OwnableUpgradeable/Ownable contract and their ownership can be transferred in a single-step process. The address the ownership is changed to should be verified to be active or willing to act as the owner.

It is recommended to implement a two-step process where the owner nominates an account and the nominated account needs to call an acceptOwnership function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. This can be achieved by using OpenZeppelin’s Ownable2StepUpgradeable contract instead of the OwnableUpgradeable.

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

The project is using Solidity version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice will enhance code readability and make the purpose of each mapping more explicit.

Consider refactoring the mappings to use named arguments.

For example, on instead of declaring:
mapping(uint256 => string) public tokenIdToIdentity;
The mapping could be declared as:
mapping(uint256 tokenId => string identity) public tokenIdToIdentity;

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

All the Shinkai smart contracts are marked as unlicensed, as indicated by the SPDX license identifier at the top of the files:

// SPDX-License-Identifier: UNLICENSED

Using unlicensed contract can lead to legal uncertainties and potential conflicts regarding the usage, modification and distribution rights of the code. This may deter other developers from using or contributing to the project and could potentially lead to legal issues in the future.

It is recommended to choose and apply an appropriate open-source license to the smart contracts. Some popular options for blockchain and smart contract projects include:

1. MIT License: A permissive license that allows for reuse with minimal restrictions.

2. GNU General Public License (GPL): A copyleft license that ensures derivative works are also open-source.

3. Apache License 2.0: A permissive license that provides an express grant of patent rights from contributors to users.

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

The unclaimIdentity function implemented in ShinkaiRegistry calls claimStakingRewards method before proceeding with the unclaim process to ensure the user's staking rewards are accrued. After accruing, it burn the related NFT. 

    function unclaimIdentity(string calldata identity) public onlyOwnerIdentity(identity) {
        claimStakingRewards(identity);

        shinkaiNft.burn(identityData[identity].boundNft);
        emit IdentityUnclaim(identity, identityData[identity].boundNft);

        delete tokenIdToIdentity[identityData[identity].boundNft];
        delete identityData[identity].boundNft;
        _resetIdentityData(identity);

        shinToken.transfer(msg.sender, identityData[identity].stakedTokens);
        identityData[identity].stakedTokens = 0;
        emit StakeUpdate(identity, 0);
        identityData[identity].lastUpdated = block.timestamp;
    }

The burn function in ShinkaiNft triggers the _beforeTokenTransfer method, which then calls back to the registry contract, executing the claimRewards function.

    function _beforeTokenTransfers(address from, address, /*to*/ uint256 startTokenId, uint256 /*quantity*/ )
        internal
        override
    {
        if (from == address(0)) return;
        string memory identity = registry.tokenIdToIdentity(startTokenId);
        registry.claimRewards(identity);
    }

The claimRewards function accumulates both staking and delegation rewards.

    function claimRewards(string memory identity) public returns (uint256 tokensAccrued) {
        tokensAccrued += claimStakingRewards(identity);
        tokensAccrued += claimDelegationRewards(identity);
    }

Since the claimRewards function already includes a call to claimStakingRewards, the initial call to claimStakingRewards in the unclaimIdentity function is redundant and can be removed to optimize gas costs.

Consider removing the redundant invocation.

ACKNOWLEDGED: The Shinkai team acknowledged this finding.

The files in scope currently use floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.0, and less than 0.9.0. It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Additionally, using a newer compiler version that introduces default optimizations, including unchecked overflow for gas efficiency, presents an opportunity for further optimization.

Lock the pragma version to the same version used during development and testing.

ACKNOWLEDGED: The Shinkai team acknowledged this finding.