CertiK reviewed Haven1 (Haven1 - Permission Manager) on 4 Mar 2025: 22 findings, 4 critical; 16 marked fixed, 6 open.
Report hosted on skynet.certik.com.
Trustblock does not review code.
Statuses are the security firm’s own, from its report.
| Severity | Open | Acknowledged | Fixed |
|---|---|---|---|
| Critical | 0 | 0 | 4 |
| High | 0 | 0 | 1 |
| Medium | 0 | 0 | 2 |
| Low | 6 | 0 | 9 |
The issue occurs when the `permissionsInterface.setPolicy("_nwAdminOrg", "NONE", "_oAdminRole")` function is called, which assigns the `"NONE"` role to the `_nwAdminRole`. This leads to unintended behavior in the system, as the role `"NONE"` is effectively treated as a valid role, bypassing proper access control mechanisms. Specifically, when calling `permissionsInterface.isNetworkAdmin(address(0xDEADBEEF))`, it incorrectly returns **true**, granting network admin privileges to any address assigned the `"NONE"` role. This allows unauthorized users to gain network admin status, undermining the security model of the application. The issue is caused by improper handling of the `"NONE"` role, which should not grant administrative privileges.
The issue arises because the role `"NONE"` is being assigned through the function `permissionsInterface.addNewRole("NONE","_orgId",3,true,true)`, which inadvertently bypasses the intended role-based permission checks. When the `"NONE"` role is assigned, it causes the function `permissionsInterface.isOrgAdmin(address(0xDEADBEEF),"_orgId")` to always return true for the given address (e.g., `0xDEADBEEF`). This could allow unauthorized users or addresses to gain admin privileges, effectively bypassing the access control mechanism and potentially compromising the system's security. The issue is caused by the improper handling of the `"NONE"` role, leading to unintended behavior in the role validation logic.
The smart contract suffers from a lack of access control, allowing any user to invoke sensitive functions such as: - `init()` - `setPolicy()` - `addAdminNode()` - `addAdminAccount()` - `updateNetworkBootStatus()` These functions are critical for managing admin nodes, accounts, and policies, and their unrestricted accessibility poses severe security risks. An attacker could exploit this flaw to gain unauthorized privileges, disrupt the system, or compromise its integrity, potentially resulting in financial losses, operational failures, and reputational damage.
The issue lies in using the zero address (**0x0**) in the `permissionsInterface.isOrgAdmin(address(0x0), "_orgId")` function call. The problem occurs because the function incorrectly treats the zero address as a valid organization admin. This is unintended behavior, as the zero address should never be considered a legitimate admin. The use of **0x0** in this context bypasses proper validation and can potentially grant unauthorized access, leading to security risks in the system. The function should include checks to prevent the zero address from being mistakenly recognized as an admin.
The issue with the `getNodeDetails()` function is that it relies on the nodeIdToIndex mapping, which is never initialized. Specifically, the check if `nodeIdToIndex[keccak256(abi.encode(_enodeId))] == 0` always returns **true** because the `nodeIdToIndex` mapping contains no entries, and thus the default value for any uninitialized key is **0**. As a result, the function fails to perform its intended logic and effectively becomes broken or useless
The issue arises from inconsistent handling of the active flag and the `votingStatus` mapping. ```sol=179 for (uint256 i = 0; i < orgVoterList[id].voterList.length; i++) { if (orgVoterList[id].voterList[i].active) { orgVoterList[id].votingStatus[id][ orgVoterList[id].voterList[i].vAccount ] = false; } } ``` When the voting item is initialized the `votingStatus` of each `active` voter is reset to `false`. However, when the inactive voter is activated via `addVoter()`, its `votingStatus` is not updated and can still be `true`. This leads to inconsistency.
The issue lies in the incorrect usage of `keccak256(abi.encode(abi.encode(_roleId)))` instead of the intended `keccak256(abi.encode(_roleId))` in the protection check. This misprint causes the condition to fail in certain scenarios, as the double `abi.encode()` results in incorrect hashing. As a result, the function fails to properly prevent the assignment of the "orgAdminRole", allowing unauthorized role assignments.
If an event is not indexed in a smart contract, it means that the event's parameters are not tagged with the `indexed` keyword. This has implications for how the event data can be searched and filtered when looking through blockchain logs. Without indexing, the event will still emit the data as part of the transaction log, but users won't be able to query for these events using the parameters. They'll have to retrieve the entire set of logs and manually sift through them to find events with the specific data. This can be less efficient and more time-consuming, especially on a blockchain with a high volume of transactions and events.
One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. ```solidity=295 function approveNode( ``` `approveNode` has memory location parameters: `_enodeId`, `_ip`, `_orgId`. ```solidity=150 function addSubOrg( ``` `addSubOrg` has memory location parameters: `_pOrgId`, `_orgId`, `_enodeId`, `_ip`. ```solidity=674 function startBlacklistedNodeRecovery( ``` `startBlacklistedNodeRecovery` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=301 function approveOrg( ``` `approveOrg` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=599 function addNode( ``` `addNode` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=123 function approveOrg( ``` `approveOrg` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=254 function addNode( ``` `addNode` has memory location parameters: `_enodeId`, `_ip`, `_orgId`. ```solidity=70 function addRole( ``` `addRole` has memory location parameters: `_roleId`, `_orgId`. ```solidity=337 function updateNodeStatus( ``` `updateNodeStatus` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=267 function addOrg( ``` `addOrg` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=348 function checkOrgActive(string memory _orgId) public view returns (bool) { ``` `checkOrgActive` has memory location parameters: `_orgId`. ```solidity=234 function addAdminNode( ``` `addAdminNode` has memory location parameters: `_enodeId`, `_ip`, `_orgId`. ```solidity=797 function assignAccountRole( ``` `assignAccountRole` has memory location parameters: `_orgId`, `_roleId`. ```solidity=463 function connectionAllowed( ``` `connectionAllowed` has memory location parameters: `_enodeId`, `_ip`. ```solidity=306 function addNode( ``` `addNode` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=96 function addOrg( ``` `addOrg` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=363 function startBlacklistedNodeRecovery( ``` `startBlacklistedNodeRecovery` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=387 function approveBlacklistedNodeRecovery( ``` `approveBlacklistedNodeRecovery` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=274 function addOrgNode( ``` `addOrgNode` has memory location parameters: `_enodeId`, `_ip`, `_orgId`. ```solidity=630 function updateNodeStatus( ``` `updateNodeStatus` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=706 function approveBlacklistedNodeRecovery( ``` `approveBlacklistedNodeRecovery` has memory location parameters: `_orgId`, `_enodeId`, `_ip`. ```solidity=58 function addAdminNode( ``` `addAdminNode` has memory location parameters: `_enodeId`, `_ip`. ```solidity=344 function updateNodeStatus( ``` `updateNodeStatus` has memory location parameters: `_enodeId`, `_ip`, `_orgId`.
An SPDX (Software Package Data Exchange) license identifier is a standardized short-form identifier for software licenses. These identifiers are used to communicate the licensing components of software packages in a concise, consistent, and machine-readable manner. Placing the SPDX license identifier on the first line of a file is a best practice because it enhances visibility, ensures consistency, supports tool compatibility, and aids in standardizing license documentation across codebases. However, in the file, the SPDX license identifier is misplaced, thus the best practice is not followed. For more references: 1. [SPDX Official Website](https://spdx.org) 2. [GitHub Licensing a Repository Help Page**](https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/licensing-a-repository) 3. [Open Source Initiative (OSI)](https://opensource.org/licenses/MIT) 4. [SPDX License List](https://spdx.org/licenses/)
The cited address input is missing a check that it is not `address(0)`.
When attempting to index dynamic data types like `string`, `bytes`, `array`, or `struct` in Solidity, they don't get stored in their original form. Instead, the Ethereum log system stores the `Keccak-256` hash of these data types. While this approach ensures efficiency and cost-effectiveness, developers must be aware of it to correctly use and interpret logs, and they cannot retrieve the original string from its hash alone.
In the provided Solidity function, there is a redundant `boolean` check for `OrgIndex[keccak256(abi.encodePacked(_orgId))] != 0`. This condition is already evaluated earlier in the function, where it returns `false` if `OrgIndex[keccak256(abi.encodePacked(_orgId))] == 0`. Since the function would exit early in such cases, repeating this check in the return statement is unnecessary. This redundancy increases gas consumption and complicates the code without adding any functional value. Removing the repeated check can improve both efficiency and code readability.
The linked statements do not affect the functionality of the codebase and appear to be either leftovers from test code or older functionality.
State variable that never change should be declared as `constant` to save gas. ```solidity=27 uint256 private fullAccess = 3; ``` - `fullAccess` should be declared `constant`.
Some state variables are not used in the codebase. This can lead to incomplete functionality or potential vulnerabilities if these variables are expected to be utilized. - Variable `networkBoot` in `OrgManager` is never used in `OrgManager`. - Variable `adminOrgId` in `OrgManager` is never used in `OrgManager`. - Variable `permUpgradable` in `PermissionsInterface` is never used in `PermissionsInterface`.
The report names a repository or files. Open the report to see its scope.
Repository and files the report names (8)
1,044 projects reviewed · 1,375 reports · latest May 2025
See CertiK’s auditsSomething wrong on this page? A missing report, a wrong date, a finding that was fixed since. Tell us and we check it against its source.
Tell us