CyberConnect - Audit

In the contract `Soul`, 
the role `owner` has authority over the functions listed below.
- `setMinter`: set/update minter status for an address.
- `setTokenURI`: update token uri.
- `transferOwnership`: set a new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.

Any compromise to the `owner` account may allow a hacker to take advantage of this authority, update minter status and modify uri.


the role `minter` has authority over the functions listed below.
- `createSoul`: mint soul token.
- `setOrg`: update org status for address.
- `clearGatedMetadatas`: clear gated metadata on a token.
- `batchSetGatedMetadatas`: batch set gated metadata.

Any compromise to the `minter` account may allow a hacker to take advantage of this authority issue and modify soul token.

---

In the contract `CyberEngine`, 
the role soul token owner has authority over the functions listed below.
-  `setOperatorApproval`: set operator for the self(soul token owner).

the role soul token owner and corresponding operator have authority over the functions listed below.
- `registerEssence`: register essence and create essence contract for soul token owner.
- `registerSubscription`: register subscribe and create subscribe contract for soul token owner (can only be called once).
- `publishContent`: publish content token set for soul token owner with content parameters.
- `share`: publish content token set for soul token owner with share parameters.
- `comment`: publish content token set for soul token owner with comment parameters.
- `setEssenceData`: update essence middleware and configuration.
- `setSubscriptionData`: update subscription configuration.
- `setContentData`: update content configuration and middleware.
- `setW3stData`: update W3st configuration and middleware.

When the soul token owner is labeld as org account in the `soul` contract, the soul token owner and corresponding operator have authority over the functions listed below.
- `issueW3st`: issue W3st token set for collect.

Any compromise to the `soul` owner account corresponding operator may allow a hacker to take advantage of this authority issue and manipulate user's functionality in `CyberEngine`, like creating unexpected content.

---

In the contract `Essence`, the role `Engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
- `mint`: mint Essence token.


In the contract `Subscribe`, the role `Engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
- `mint`: mint Subscribe token
- `extend`: extend the expiration date for one token.


In the contract `Content`, the role `Engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
- `mint`: mint Content token.


In the contract `W3st`, the role `Engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
- `mint`: mint W3st token.

---

In the contract `MiddlewareManager`, the role `owner` has authority over the functions listed below.
- `allowMw`: update the status for the middleware address.
- `transferOwnership`: set a new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.

Any compromise to the owner's account may allow a hacker to take advantage of this authority issue and manipulate issue invalid middleware for malicious purposes.

---

In the contract `TokenReceiver` the role `owner` has authority over the functions listed below.

- `withdraw`: send native token from contract to arbitrary address
- `transferOwnership`: set new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.

Any compromise to the owner's account may allow a hacker to take advantage of this authority issue and withdraw the native token.
  

In the contract `PermissionMw`, the role `engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
-  `setMwData`: set `_signerStorage` information
-  `preProcess`: validate collector signature

In the contract `LimitedTimePaidMw`, the role `engine` has authority over the functions listed below. Based on design the Engine address should be the contract `CyberEngine`:
-  `setMwData`: set up limited time paid parameters
-  `preProcess`: validate collector signature

In the contract `Treasury` the role `owner` has authority over the functions listed below.

- `setTreasuryAddress`: set new treasury address
- `setTreasuryFee`: set new treasury fee
- `allowCurrency`: update the currency status
- `transferOwnership`: set new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.

Any compromise to the owner's account may allow hackers to take advantage of this authority issue and update treasure settings.

---

In the contract `CyberAccountFactory` th

In the contract `CyberId`, the role `owner` has authority over the functions listed below.
- `setBaseTokenUri`: set token uri
- `setMiddleware`: set middleware
- `transferOwnership`: set new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.
- `clearGatedMetadatas`: clear gated metadata on a token.
- `batchSetGatedMetadatas`: batch set gated metadata(when id not is issued or when id is not expired).

Any compromise to the `owner` account may allow a hacker to take advantage of this authority and manipulate the contract setting.

---
Note: Update in Commit [26aa0e00eeb15bec52c77abc011be949d2f13e09](https://github.com/cyberconnecthq/cyberid/commit/26aa0e00eeb15bec52c77abc011be949d2f13e09)
The contract `MocaId` was renamed to `RealmId`.

In the contract `RealmId`(`MocaId`), multiple roles have authority over the functions list below.
- `DEFAULT_ADMIN_ROLE`:
  - `setbaseTokenURI`: set base token uri
  - `setMiddleware`: set middleware address
  - `pause`: pause the contract
  - `unpause`: unpause the contract
  - `grantRole`:  assign `PAUSER_ROLE`, `UPGRADER_ROLE`, `MINTER_ROLE` and `DEFAULT_ADMIN_ROLE` to new address.
  - `revokeRole`: revoke `PAUSER_ROLE`, `UPGRADER_ROLE`, `MINTER_ROLE` and `DEFAULT_ADMIN_ROLE` from address.
- `OPERATOR_ROLE`:
  - `allowNode`: set allowed node/extension
  - `setMocaXP`: sets the moca xp
  - `clearGatedMetadatas`: clear gated metadata on a token
  - `batchSetGatedMetadatas`: batch set gated metadata(when id not is issued or when id is not expired).

Any compromise to the aforementioned roles may allow a hacker to take advantage of this authority and manipulate the MocaId's setting.

Update in Commit [26ba7731e79531edf4b1414c730b292672c4b6a9](https://github.com/cyberconnecthq/cyberid/commit/26ba7731e79531edf4b1414c730b292672c4b6a9)

The contract using the `owner` role to control the aforementioned functions:
- `setbaseTokenURI`: set base token uri
- `setMiddleware`: set middleware address
- `pause`: pause the contract
- `unpause`: unpause the contract
- `allowNode`: set allowed node/extension
- `clearGatedMetadatas`: clear gated metadata on a token
- `batchSetGatedMetadatas`: batch set gated metadata
- `transferOwnership`: set new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.

---

In the contract `StableFeeMiddleware`, the role `NAME_REGISTRY` has authority over the functions listed below. The name `NAME_REGISTRY` suppose to be one of the name registry contracts.
- `setMwData`: update the information for `StableFeeMiddleware`.

---

In the contract `TrustOnlyMiddleware`: 
the role `owner` has authority over the functions listed below.
- `preRegister`: pre-check for CyberId registration.
- `preRenew`: pre-check for CyberId renew.
- `preBid`: pre-check for CyberId bid.
- `transferOwnership`: set new owner for the contract.
- `renounceOwnership`: set address(0) as the new owner.
Any compromise to the `owner` account may allow a hacker to take advantage of this authority, manipulate the contract setting and bypass the validation for id minting.

the role `NAME_REGISTRY` has authority over the functions listed below. The name `NAME_REGISTRY` suppose to be one of the name registry contracts.
- `setMwData`: update the owner for `TrustOnlyMiddleware`.

---

In the contract `PermissionMw`, the role `NAME_REGISTRY` has authority over the functions listed below. The name `NAME_REGISTRY` suppose to be one of the name registry contracts.
-  `setMwData`: set `_signerStorage` information
-  `preProcess`: validate preprocess signature

In the contract `RealmId`(`MocaId`), the role `DEFAULT_ADMIN_ROLE` has the authority to update the implementation contract behind the `MocaId` contract. 

Any compromise to the `DEFAULT_ADMIN_ROLE` account may allow a hacker to take advantage of this authority and change the implementation contract which is pointed by proxy and therefore execute potential malicious functionality in the implementation contract.

Note: Update in Commit [26ba7731e79531edf4b1414c730b292672c4b6a9](https://github.com/cyberconnecthq/cyberid/commit/26ba7731e79531edf4b1414c730b292672c4b6a9) The contract uses the `owner` role to control the pause functionality.

---

In the contract `CyberEngine`, the role `admin` has the authority to update the implementation contract behind the `CyberEngine` contract. 

Any compromise to the `admin` account may allow a hacker to take advantage of this authority and change the implementation contract which is pointed by proxy and therefore execute potential malicious functionality in the implementation contract.

If `TrustOnlyMiddleware` is used as a middleware in the contract `CyberId`, the role `_owner` has the authority to register/renew/bid CyberIds for free.

Any compromise to the `_owner` account may allow a hacker to take advantage of this authority and register/renew/bid any amount of CyberIds at will.

```solidity=185
contract TrustOnlyMiddleware is Ownable, LowerCaseCyberIdMiddleware {
//...
    function preRegister(
        DataTypes.RegisterCyberIdParams calldata params,
        bytes calldata
    ) external payable override returns (uint256) {
        require(params.msgSender == owner(), "NOT_TRUSTED_CALLER");
        return 0;
    }

    /// @inheritdoc ICyberIdMiddleware
    function preRenew(
        DataTypes.RenewCyberIdParams calldata params,
        bytes calldata
    ) external payable override returns (uint256) {
        require(params.msgSender == owner(), "NOT_TRUSTED_CALLER");
        return 0;
    }

    /// @inheritdoc ICyberIdMiddleware
    function preBid(
        DataTypes.BidCyberIdParams calldata params,
        bytes calldata
    ) external payable override returns (uint256) {
        require(params.msgSender == owner(), "NOT_TRUSTED_CALLER");
        return 0;
    }
}
```

In the contract `RealmId`(`Mocald.sol`), the role `admin` has the authority to update the status of the `_paused` and further pause/resume the functionality of the `transferFrom()`, `safeTransferFrom()` and `safeTransferFrom()` functions, which effectively impact token transfers.

Note: Update in Commit [26ba7731e79531edf4b1414c730b292672c4b6a9](https://github.com/cyberconnecthq/cyberid/commit/26ba7731e79531edf4b1414c730b292672c4b6a9) The contract uses the `owner` role to control the pause functionality.


Similarily, the owner of `CyberEngine` also can pause the Content, Essence, and W3st related functionalities in Cybergraph.


Any compromise to the private key of the `owner` may allow hackers to take advantage of this authority and allow/prevent external user access to token transfer functionalities.

In both project `cyberId` and `cyberGraph`, the function `_chargeAndRefundOverPayment` or equivalent logic is being used to return the funds for the overpayment. Most of the return funds processes are triggered in the middle of the transaction but the actual data value has not been updated yet, for example, in middleware pre-process, which will trigger the external call for the funds transfer, which will violate the check-effect-interaction pattern.

When adding/updating limited time paid information, the function `setMwData` will validate the relationship between the start and end timestamp:
```solidity=119
  require(endTimestamp > startTimestamp, "INVALID_TIME_RANGE");
```
However, there is no validation between the current and start/end timestamps. As a result, the input information can be invalid.

The cited functions do not check if the inputs are valid.
- The length of `uri` in `setEssenceData()`, `setContentData()`, `setW3stData()`, `setBaseTokenUri()`, `setbaseTokenURI()`, and `setTokenURI()` should be greater than zero.

Moreover, the function `setSubscriptionData()` does not check if the inputs are valid values.
- the length of `tokenURI` should be greater than zero
- `recipient` should be a nonzero address
- `pricePerSub` and `dayPerSub` should be greater than zero

There is no storage gap preserved in the logic contract. Any logic contract that acts as a base contract that needs to be inherited by other upgradeable child should have a reasonable size of storage gap preserved for the new state variable introduced by the future upgrades.

The following functions do not check if the input is equal to the current value, an equivalent value can return directly without updating the state to save gas.
- `function allowMw(address mw, bool allowed)`
- `function setTreasuryAddress(address treasuryAddress)`
- `function setTreasuryFee(uint16 treasuryFee)`
- `function allowCurrency(address currency, bool allowed)`

Functions that update state variables should emit relevant events as notifications.

The function `createAccount` will compute the deploy address first and try to create the contract with the same salt. However, there is no validation between computed address and deployed address to ensure the deployment is successful.

Addresses are not validated before assignment or external calls, potentially allowing the use of zero addresses and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens.

```solidity=44
        payable(to).transfer(amount);
```
- `to` is not zero-checked before being used.

The cited addresses are missing a check that they are not `address(0)`.
- `_owner`
- `_oracleAddress`
- `_recipient`
- `_entryPoint` and `_soul` in `CyberAccountFactory`

The contract `content` and `W3st` provide `safeBatchTransferFrom`, which will try to query the transferability for each token then do the batch transfer.

```solidity=84
    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] calldata ids,
        uint256[] calldata amounts,
        bytes calldata data
    ) public virtual override {
        for (uint256 i = 0; i < ids.length; i++) {
            if (
                !ICyberEngine(ENGINE).getW3stTransferability(_account, ids[i])
            ) {
                revert("TRANSFER_NOT_ALLOWED");
            }
        }

        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }
```

However, if one of the token in the list is not transferrable, the whole transaction will be reverted. As a result the user may need to create a transaction with updated input.

The function `_disableInitializers()` is already called in the parent contracts, which can ensure that the derived contract is initialized. There is no need to call it again in the derived contracts, this could save gas and make the code more efficient.

The project contains `Owned` contract definitions that is not used, which can lead to unnecessary complexity and reduced maintainability.
```solidity
abstract contract Owned is Initializable {
//...
}
```

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring native tokens, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, EVM instructions' gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation.

```solidity=44
        payable(to).transfer(amount);
```