The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug free status. The audit documentation is for discussion purposes only. The information presented in this report is confidential and privileged. If you are reading this report, you agree to keep it confidential, not to copy, disclose or disseminate without the agreement of the Client. If you are not the intended recipient(s) of this document, please note that any disclosure, copying or dissemination of its content is strictly forbidden.
A group of auditors are involved in the work on the audit. The security engineers check the provided source code independently of each other in accordance with the methodology described below:
- Project documentation review.
- General code review.
- Reverse research and study of the project architecture on the source code alone.
- Build an independent view of the project's architecture.
- Identifying logical flaws.
- Manual code check for vulnerabilities listed on the Contractor's internal checklist. The Contractor's checklist is constantly updated based on the analysis of hacks, research, and audit of the clients' codes.
- Code check with the use of static analyzers (i.e Slither, Mythril, etc).
Eliminate typical vulnerabilities (e.g. reentrancy, gas limit, flash loan attacks etc.).
- Detailed study of the project documentation.
- Examination of contracts tests.
- Examination of comments in code.
- Comparison of the desired model obtained during the study with the reversed view obtained during the blind audit.
- Exploits PoC development with the use of such programs as Brownie and Hardhat.
Detect inconsistencies with the desired model.
- Cross check: each auditor reviews the reports of the others.
- Discussion of the issues found by the auditors.
- Issuance of an interim audit report.
- Double-check all the found issues to make sure they are relevant and the determined threat level is correct.
- Provide the Client with an interim report.
- The Client either fixes the issues or provides comments on the issues found by the auditors. Feedback from the Customer must be received on every issue/bug so that the Contractor can assign them a status (either "fixed" or "acknowledged").
- Upon completion of the bug fixing, the auditors double-check each fix and assign it a specific status, providing a proof link to the fix.
- A re-audited report is issued.
- Verify the fixed code version with all the recommendations and its statuses.
- Provide the Client with a re-audited report.
- The Customer deploys the re-audited source code on the mainnet.
- The Contractor verifies the deployed code with the re-audited version and checks them for compliance.
- If the versions of the code match, the Contractor issues a public audit report.
- Conduct the final check of the code deployed on the mainnet.
- Provide the Customer with a public audit report.
All vulnerabilities discovered during the audit are classified based on their potential severity and have the following classification:
| Severity | Description |
|---|---|
| Critical | Bugs leading to assets theft, fund access locking, or any other loss of funds. |
| High | Bugs that can trigger a contract failure. Further recovery is possible only by manual modification of the contract state or replacement. |
| Medium | Bugs that can break the intended contract logic or expose it to DoS attacks, but do not cause direct loss funds. |
| Low | Bugs that do not have a significant immediate impact and could be easily fixed. |
Based on the feedback received from the Customer regarding the list of findings discovered by the Contractor, they are assigned the following statuses:
| Status | Description |
|---|---|
| Fixed | Recommended fixes have been made to the project code and no longer affect its security. |
| Acknowledged | The Customer is aware of the finding. Recommendations for the finding are planned to be resolved in the future. |
The audited scope encompasses several smart contracts designed to deposit ETH to validators and integrate with the SSV Network project. The P2pSsvProxyFactory is responsible for deploying the P2pSsvProxy contract, serving as the entry point for creating the initial deposit of validator stakes and their registration in the SSV Network. Meanwhile, the P2pSsvProxy contract is dedicated to interacting with the SSV Network's functions and managing operator clusters.
| Title | Description |
|---|---|
| Client | P2P.org |
| Project name | SSV Integration |
| Timeline | November 14 2023 - July 02 2024 |
| Number of Auditors | 3 |
| Date | Commit Hash | Note |
|---|---|---|
| 14.11.2023 | 9dd4728002d9c275e29e8ba38bcf7d90efc7531b | Commit for the audit |
| 27.11.2023 | 88e3ecae57e70410ef0ea482ba10d8f541aeac59 | Commit for the re-audit |
| 19.06.2024 | cfd0a114c760bc4d87121b1c38893f2110f0d474 | Commit for the diff-audit |
| 28.06.2024 | dfc3e024a3f0779642b43d39ca321cbcf0eb8605 | Commit for the re-audit #2 |
The audit covered the following files:
Commit 88e3ecae57e70410ef0ea482ba10d8f541aeac59 (previous version)
| Contract | Address | Comment |
|---|---|---|
| P2pSsvProxyFactory | 0x10f4ec919e3e692cb79301e58a7055c783630dfc | |
| P2pSsvProxy | 0xbd057f7778485da79c40b252bb09cf1f163d2365 |
Commit dfc3e024a3f0779642b43d39ca321cbcf0eb8605 (latest version)
| Contract | Address | Comment |
|---|---|---|
| P2pSsvProxyFactory | 0xcb924D4BE3Ff04B2d2116fE116138950373111d9 | |
| P2pSsvProxy | 0xec17A02B2A8b0C291C0DddE2a00Ca24477c17ED5 |
| Severity | # of Findings |
|---|---|
| CRITICAL | 0 |
| HIGH | 1 |
| MEDIUM | 1 |
| LOW | 9 |
During the audit, 1 high, 1 medium, and 4 low severity findings have been discovered, confirmed, and either fixed or acknowledged by the developers. An acknowledged finding does not impact the overall security of the project.
During the additional diff audit (comparing commit cfd0a114c760bc4d87121b1c38893f2110f0d474 with base commit 88e3ecae57e70410ef0ea482ba10d8f541aeac59), the following attack vectors have been checked:
-
Verification of the correctness of adding/removing validators via
bulkRegisterValidator/bulkRemoveValidator:- The code review of the
bulkRegisterValidatorandbulkRemoveValidatorfunctions confirmed that the logic for adding and removing validators is implemented correctly. The functions include checks to ensure that validators are added and removed as intended. No vulnerabilities or logical errors were identified.
- The code review of the
-
Testing the addition of two validators with the same keys:
- The code analysis to check the handling of validator key uniqueness showed that the code contains mechanisms to prevent the addition of duplicate validators with the same keys. Appropriate error handling is in place. No vulnerabilities were found.
-
Correctness of Access Rights Handling
- The code review focused on the handling of access rights across various functions and modules. The analysis confirmed that access control mechanisms are correctly implemented, with appropriate checks to ensure that only authorized entities can perform specific actions.
The code review during the diff audit confirms that the examined components and integrations are secure and implemented correctly according to the specified standards. No critical, high or medium severity vulnerabilities were identified during the analysis. Only findings affecting code optimization and readability, but not security, were found.
Not found
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/67086f60eae25eeb5abfec43920c4b8346a2fe09
This issue has been identified in the depositEthAndRegisterValidators function of the P2pSsvProxyFactory contract.
This function permits transferring any specified amount of SsvToken to the newly created SsvCluster in the SsvNetwork. The issue arises from the lack of restrictions on the SsvPayload.tokenAmount parameter, enabling users to drain the entire SsvToken balance by depositing 32 ETH. Consequently, these tokens can only be recovered through a multi-step process where the owner has to withdraw the total amount from the SsvCluster and then transfer it from the corresponding P2pSsvProxy back to P2pSsvProxyFactory. Moreover, the attacker retains control over their deposit and can withdraw it using their specified withdrawCredentials.
This vulnerability is classified as high severity due to its potential to block deposits from subsequent clients until the owner intervenes.
We recommend limiting the maximum value of SSVToken tokens transferred to a reasonable amount to prevent system block due to actions of unprivileged users and to enhance the system's overall security.
1. Inability to revoke rights given in setAllowedSelectorsForClient and setAllowedSelectorsForOperator
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/b96c5ab85da150ea3e23dae8502b457715192917
The issue is found in setAllowedSelectorsForClient and setAllowedSelectorsForOperator functions of P2pSsvProxyFactory contract.
These functions currently grant access rights for invoking SsvNetwork functions directly by client and operator through P2pSsvProxy.fallback but do not provide a mechanism to revoke these rights. This shortfall presents a significant security risk, especially in scenarios where excessive permissions are incorrectly assigned by the owner. Additionally, the ssvNetwork is an upgradeable proxy contract, and the inability to revoke rights in the event of an interface change further increases the vulnerability.
This issue is classified as medium due to the risks associated with the irreversibility of incorrectly granted access.
To mitigate this risk, it is recommended to introduce onlyOwner functions that enable the revocation of rights for both the client and operator in invoking specific functions of the SsvNetwork through P2pSsvProxy.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/a20c857b2ebb283fa2a7a491bab062c9f1f81dac
This issue is found in the fallback function of the P2pSsvProxy contract.
The current implementation emits an event before a revert operation. However, it's important to note that a transaction revert fully reverses all state changes made during the transaction, including the event log. This renders the event emission redundant and ineffective, as it will be erased upon the execution of revert.
We recommend removing this event emission.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/901b78e16ac215903888ea91e8eb24d5a5eb12e0
This issue is found in the registerValidators function if the P2pSsvProxy contract.
This function, which is only accessible from the factory, is invoked within the P2pSsvProxyFactory._registerValidators function. Before this invocation, a validation check confirms that the P2pSsvProxy has been created with a specific feeDistributorInstance. This fee distributor is immutable and is assigned as the s_feeDistributor storage variable within P2pSsvProxy. Given this setup, explicitly passing the feeDistributor parameter to registerValidators is unnecessary.
We recommend removing the feeDistributor parameter from the registerValidators function and utilizing the existing s_feeDistributor storage variable instead.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/20b6a086af234ef369b5f327981e8abe7e2930fc
A gas efficiency issue is found in the _makeBeaconDeposit function of the P2pSsvProxyFactory contract.
The problem arises because the withdrawCredentials variable is repeatedly assigned the same value within a loop. This repetitive assignment is unnecessary and increases gas costs for executing this function.
We recommend moving the declaration of the withdrawCredentials variable outside the loop.
Acknowledged
This issue is found in the setSsvPerEthExchangeRateDividedByWei function of the P2pSsvProxyFactory contract.
Currently, the contract owner can arbitrarily set the exchange rate of SsvToken to ETH within given limits. This poses the risk of potential errors by the owner.
We recommend integrating an oracle for the SsvToken to derive its market price from real-time market data.
This is a good idea. But we deliberately decided to do it the simple way and accept the risk to ourselves. It's important that the client is not risking here. They see the exchange rate and proceed only if they agree.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
While the project is using Solidity version 0.8.24, it is redundant to use unchecked incrementing of the for-loop variables since Solidity implements such behavior by default starting from version 0.8.22.
We recommend using a standard for-loop increment notation for better code readability.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
Acknowledged
Currently, custom errors are implemented using the if(condition) revert(CustomError) flow, which is the only way to implement custom errors in Solidity versions before 0.8.26. However, the latest version of Solidity already supports the require(bool, CustomError) flow.
We recommend considering upgrading Solidity to the latest version and replacing the if - revert flow with the require flow when it is beneficial for code readability.
We prefer staying on 0.8.24 to match SSV Solidity version exactly. Switched to evm_version = 'cancun' for the same reason.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
While the constant value MAX_ALLOWED_SSV_OPERATOR_IDS was updated from 16 to 24, the corresponding comment limited to 16 IDs in the code was not updated.
We recommend updating the comment to keep it consistent with the code.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
The values passed to the makeBeaconDepositsAndRegisterValidators function are validated twice: within the function itself and in the P2POrgUnlimitedEthDepositor.makeBeaconDeposit function. The validation in makeBeaconDepositsAndRegisterValidators can be omitted without compromising code security.
We recommend considering to remove the redundant validation from makeBeaconDepositsAndRegisterValidators.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
The s_allowedSsvOperatorIds array is stored as a static-size array with the size of MAX_ALLOWED_SSV_OPERATOR_IDS. To obtain data from that array, the current implementation generally reads the whole array from storage, regardless of whether it uses all its capacity or not. This is a suboptimal solution in terms of gas usage.
Related code:
- https://github.com/p2p-org/p2p-ssv-proxy/blob/cfd0a114c760bc4d87121b1c38893f2110f0d474/src/p2pSsvProxyFactory/P2pSsvProxyFactory.sol#L232
- https://github.com/p2p-org/p2p-ssv-proxy/blob/cfd0a114c760bc4d87121b1c38893f2110f0d474/src/p2pSsvProxyFactory/P2pSsvProxyFactory.sol#L276
We recommend developing a more optimal solution to save gas.
Fixed in https://github.com/p2p-org/p2p-ssv-proxy/commit/dfc3e024a3f0779642b43d39ca321cbcf0eb8605.
MixBytes is a team of blockchain developers, auditors and analysts keen on decentralized systems. We build opensource solutions, smart contracts and blockchain protocols, perform security audits, work on benchmarking and software testing solutions, do research and tech consultancy.