CyberConnect

CertiK9 Sep 2022Files named in the report

Audit

CertiK reviewed Cyber (CyberConnect) on 9 Sep 2022: 18 findings, 0 critical; 15 marked fixed, 3 open.

No open critical findings

Open the report Open Cyber’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings18

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	2	0
Medium	0	2
Low	1	13

MediumPotential Reentrancy AttackFixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If an attacker controls the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. In this case the functions `subscribe()`, `subscribeWithSig()`, `collect()`, and `collectWithSig()` can all be reentered as they use the `_safeMint()` function which calls `onERC721Received()` if the address `to` has any code (in which case it is a contract). This checks the contract intends to be able to receive `ERC721` tokens, but this function can also be coded to make a recursive call. Currently, the only issue this poses is that events can be emitted out of order. However, if middleware is introduced that uses a post-process, then the post-processes will be called out of order and may pose a security risk.

MediumWrong Variable in CheckFixed

It the function `setTreasuryFee()` in `Treasury.sol`, it is checked that `_treasuryFee <= Constants._MAX_BPS`. This checks that the current treasury fee, not the input treasury fee, is valid.

LowLack of `onlyInitializing` ModifierFixed

The functions ` __ReentrancyGuard_init()`, `__Auth_Init`, `__ERC721_Init`, and `__Owned_Init` should only be called once during contract initialization.

LowLack of input validationFixed

The function `registerEssence()` should verify the inputs `data.name`, `data.symbol`, and `data.essenceTokenURI` are not the empty string.

LowMissing Check For `v` And `s`Fixed

In `_requiresExpectedSigner()`, the `ecrecover()` function is being used. [EIP-2](https://eips.ethereum.org/EIPS/eip-2) still allows signature malleability for `ecrecover()`. Appendix F in the [Ethereum Yellow paper](https://ethereum.github.io/yellowpaper/paper.pdf), defines the valid range for s in (311): `0 < s < secp256k1n ÷ 2 + 1` and for the recovery identifier (312): `v ∈ {0,1}`. This should not be confused with the input for `ecrecover()` where `v ∈ {27,28}`. (See [doc](https://ethereum.github.io/execution-specs/autoapi/ethereum/frontier/vm/precompiled_contracts/ecrecover/index.html)) However, these values can be obtained by taking `27+"recovery identifier"`, so that they will also yield a unique signature and are often the `v` values returned from signatures. (For example [`web3.eth.accounts.sign()`](https://web3js.readthedocs.io/en/v1.7.5/web3-eth-accounts.html)) If your library generates malleable signatures, such as `s`-values in the upper range, calculate a new `s`-value with `0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1` and flip `v` from `27` to `28` or vice versa. If your library also generates signatures with `0/1` for `v` instead `27/28`, add `27` to `v` so that `ecrecover()` accepts these signatures as well.

LowMissing checks or input validationsFixed

In `upgradeSubscribeNFT()` and `upgradeEssenceNFT()` of `CyberEngine.sol`, there are no checks that the `namespace` addresses passed as inputs are valid. --- In `ProfileNFT.sol` the following functions do not check the `profileId` is minted: - `getSubscribeMw()` - `getSubscribeNFT()` - `getSubscribeNFTTokenURI()` - `getEssenceNFT()` - `getEssenceNFTTokenURI()` - `getEssenceMw()` Furthermore, the following functions do not check the `essenceId` exists: - `getEssenceNFT()` - `getEssenceNFTTokenURI()` - `getEssenceMw()` --- In the `constructor()` of `Treasury.sol`, there is no check that the input `treasuryFee` is less than or equal to `Constants._MAX_BPS`.

LowMissing Emit EventsFixed

There should always be events emitted in the sensitive functions that are controlled by centralization roles. Also, in the contract `IProfileNFTEvents.sol`, event `SetAnimationTemplate` has been defined but not emitted in any contracts.

LowMissing Error MessagesFixed

**require** can be used to check for conditions and throw an exception if the condition is not met. In addition, it can provide a string message containing details about the error that will be passed back to the caller.

LowMissing Zero Address ValidationFixed

The following addresses should be checked that they are not the zero address before assignment or external call: - In `Owned.sol`, `newOwner` in `setOwner()` function. - In `Treasury.sol`, `treasuryAddress` in the `constructor()`. - In `Treasury.sol`, `treasuryAddress` in `setTreasuryAddress()` function. - In `UgradeableBeacon.sol`, `owner` in the `constructor()`.

LowProfile UpgradabilityFixed

In `ProfileNFT.sol`, the function `_authorizeUpgrade()` has the `onlyEngine` modifier so that only the `ENGINE` can upgrade the implementation contract. However, there is no functionality in `CyberEngine.sol` that would enable it to be upgraded.

Show 5 more

LowToken with empty URIFixed

The function `tokenURI()` returns the metadata JSON object for a given `tokenId`, but returns an empty string.

LowTyposFixed

In `ISubscribeMiddleware.sol` and then also in `SubscribOnlyOnceMw.sol` there are typos in `preProcess()` and `postProcess()` function arguments: `subscrbeNFT` should be `subscribeNFT`. --- In `Constants.sol` there is a typo at line 38 in the `_SET_OPERATOR_APPROVAL_TYPEHASH` constant. The string passed to keccak256 is `setOperatorApprovalWithSign(...)`. It should be `setOperatorApprovalWithSig()`. --- In `CyberNFTBase.sol`, `EIP712.sol`, and `PermissionedFeeCreationMw.sol` the function named `_domainSeperatorName()` has a typo in it. It should be `_domainSeparatorName()`. --- In `Constants.sol`, `CyebreEngine` is written instead of `CyberEngine`. --- In `Treasury.sol`, `Treasury` should be written instead of `Treasurt`. --- In `SubscribeOnlyOnceMw.sol` and `IEssenceMiddleware.sol`, `Proccess` and `aready` are written instead of `Process` and `already` respectively. --- In `IEssenceMiddleware.sol`, `essenceeNFT` is written instead of `essenceNFT`. --- In `PermissionedFeeCreationMw.sol`, `PUBLIC VIEW` should be `EXTERNAL VIEW`. --- In `UpgradeableBeacon.sol`, the comment for the `constructor()` states the deployer account is the owner. However, the owner will be the address input to the constructor, not necessarily the deployer account. --- In `QRSVG.sol`, the function `generateQRCode()` has `emit MatrixCreated(qrMatrix.matrix);` commented-out. This event is not defined and this commented-out code can be deleted.

LowUsage of `_currentIndex`Fixed

In `_createProfile()`, `_currentIndex` is often used instead of `tokenID` when they are the same value.

LowUsage of `transfer()` for sending EtherFixed

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring Ether, 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=93 payable(mwData.recipient).transfer(actualCollected); if (treasuryCollected > 0) { payable(_treasuryAddress()).transfer(treasuryCollected); } ```

LowUsage of non-allowed middlewaresFixed

If a user sets a middleware and that middleware is later disabled with `allowEssenceMw()`, `allowSubscribeMw()`, or `allowProfileMw()` it will only disable users who have not already implemented it. For example: - `exampleMw` is allowed in `CyberEngine.allowExampleMw()`. - `exampleMw` is set through `setSubscribeData()`, `setEssenceData()`, or `setProfileMw()`. - For any reason `exampleMw` is then disabled using `CyberEngine.allowExampleMw()`. - `exampleMw` will still be used because the mapping storing the middleware is not updated and it is only checked that the middleware is allowed when setting the middleware.