Everstake is a responsible validator trusted by 625k+ users across 70+ blockchain networks created by engineers for the entire community in 2018.
Everstake 0.1+ ETH staking solution is a protocol that allows users to deposit amounts that can be less than 32 ETH. Once users’ deposits exceed 32 ETH, a new validator is created, and the users can start earning rewards. The staking rewards are restaked automatically, and the users’ pool shares are increased.
Everstake engaged Ackee Blockchain to perform a security review of the Everstake protocol (Revision 1.0 and 2.0) for a total of 20 engineering days in a period between July 3 and August 29, 2023.
Everstake also engaged Ackee Blockchain to perform a fix review of the second audit revision on commit 38970a6.
METHODOLOGY
Revision 1.0
We began our review by using static analysis tools, namely Woke. We then took a deep dive into the logic of the contracts. For testing and fuzzing, we have involved a Woke testing framework.
During the review, we paid special attention to:
- ensuring the accounting arithmetic of the system is correct
testing whether no unstaking path reverts - testing whether users can’t withdraw more than they have deposited (+ rewards)
- analyzing the management of the validators
- detecting possible ETH call reentrancies in the code
- ensuring access controls are not too relaxed or too strict
- testing if rewards are distributed according to users’ shares
- analyzing that the contracts use proper data structures for storing deposits, withdrawals, etc.
- analyzing the upgradeability pattern (storage collision, access control, etc.)
- analyzing whether the withdrawal credentials are created correctly
- looking for common issues such as data validation.
Revision 2.0
We followed the methodology established in the previous revision:
- We focused on manually reviewing all the changes and wrote additional tests in Woke.
- We wrote a new simple differential fuzz test for the quick sort function and made it available in the Woke appendix.
- We also utilized Woke for static analysis, which was mainly useful for analysis of reentrancies.
During the review, we had similar objectives as in the previous revision; additionally, we focused on:
- verifying that all the fixes were applied correctly
- withdrawing edge-case amounts (like very small values, values equalling all shares, etc.)
- minting shares and subsequent reward distribution
- reviewing the integer-division-based loss of precision introduced in the amount-share conversions
- reviewing the view functions (mainly _simulateAutocompound)
- reviewing the new ordering logic of the validators
- analyzing transaction ordering and front-running opportunities
- reviewing the new upgradeability pattern.
SCOPE
Revision 1.0
The audit has been performed on commit 60688fc, and the scope was the entire contracts folder:
contracts/ ├── Accounting.sol ├── AutocompoundAccounting.sol ├── Governor.sol ├── Pool.sol ├── RewardsTreasury.sol ├── TreasuryBase.sol ├── WithdrawTreasury.sol ├── Withdrawer.sol ├── common │ └── Errors.sol ├── interfaces │ ├── IAccounting.sol │ ├── IDepositContract.sol │ ├── IPool.sol │ ├── IRewardsTreasury.sol │ └── ITreasuryBase.sol ├── lib │ ├── UnstructuredRefStorage.sol │ └── UnstructuredStorage.sol
├── structs │ ├── ValidatorList.sol │ └── WithdrawRequests.sol └── utils ├── Math.sol └── OwnableWithSuperAdmin.sol
Revision 2.0
The review was done on commit 35f9b56 and the files in scope were the same as in the previous audit. Since the last audit 45 new commits were made, a large number of those were fix commits and refactoring commits. The most notable changes were:
- adding upgradeability to treasuries
- making interchange optional
- modifying the ordering logic of the validators
- adding gas optimizations
- improving naming of the variables and adding comments
- fixing the issues found in the previous audit.
Revision 2.1
We performed a fix review of the second audit revision on commit 38970a6.
FINDINGS
Here, we present our findings.
Revision 1.0
Critical severity
No critical severity issues were found.
High severity
H1: _simulateAutocompound can revert
H2: DoS due to 0 pending deposits
H3: Partial DoS due to interchange
H4: DoS due to underflow
Medium severity
M1: Missing whenWithdrawActive modifier
M2: Call to depositedBalanceOf reverts
Low severity
L1: Withdraw request array monotonically grows
L2: Lack of 2-phase role transfers
L3: Exiting validators can revert
L4: Replacing validators lacks validation
L5: Validation of owner in treasuries
L6: Data validation in initialize functions
L7: Incorrect return value of _simulateAutocompound
L8: Upgradeable contract constructor without initializer
L9: Insufficient data validation when composing contracts
Warning severity
W1: Usage of solc optimizer
W2: Dead code in _autoCompoundUserBalance
W3: Unchecked return of _update
W4: Lack of contract prefix in storage position
W5: Pool fee can be set extremely high
Informational severity
I1: Used library
I2: Comparison with role outside modifier
I3: Function always returns true
I4: Lack of logging in setters
I5: Code and comment discrepancy
I6: Lack of documentation
Revision 2.0
Critical severity
No critical severity issues were found.
High severity
H5: Withdrawal of autocompoundBalanceOf amount reverts
Medium severity
M3: simulateAutocompound checks only balance diff
Low severity
L10: Pending deposited can’t be withdrawn
L11: Lack of call to disableInitializers()
L12: Lack of 0 shares check in simulateAutocompound
L13: Lack of 0 shares check in feeBalance
Warning severity
W6: Withdraw can return by 1 wei more than requested
W7: Withdrawal revert due to rounding
W8: unstakePending and activateBalance can revert due to bad timing
Informational severity
I7: Code duplication for ownership
I8: Typos in code and comments
I9: Array length validation
CONCLUSION
Revision 1.0
Our review resulted in 26 findings, ranging from Info to High severity. The highest severity issues were related to denial of service and the inability to view the state of the protocol due to underflow reverts.
Overall, we do not recommend deploying the current version of the protocol. During the audit, we discovered a couple of issues that caused the protocol to revert, although the state was achieved only through normal, non-malicious transactions.
This implies that the protocol is not sufficiently tested. Additionally, we found the documentation to be lacking, so we dedicated a separate informational issue to this.
At the same time, we would like to acknowledge that the development team was able to find some of the issues independently of our review (i.e., both the teams found it independently of each other), most notably, it was H3. Additionally, during the audit, we observed multiple clever design decisions in the Pool and Accounting contracts.
However, due to the high number of issues, including high-severity ones, some work needs to be done to make the protocol production-ready.
Due to the high number of issues, lack of documentation and the fact that the protocol reverted in certain scenarios, our auditing processes were slowed down. As such, we do not feel certain that the protocol will be fully secure after the fixes are applied. We strongly recommend pursuing another audit round, though a shorter one, to ensure that the protocol is secure.
Revision 2.0
Our second review resulted in 12 findings, ranging from Info to High severity. The highest severity issue related to integer-division-based error, which in certain protocol states resulted in withdrawal reverts and thus caused temporal lock of users’ funds.
The code quality in the second revision was significantly improved. The code was more readable (mainly due to the use of better names for variables) and the documentation was better. Almost all issues from the previous revision were fixed.
Based on the observations made during the review, we recommend focusing on the following high-level objectives:
- The documentation is still lacking and could be improved.
- Due to the occurrence of another rounding-based issue, we recommend fuzzing.
- Protocol to ensure that no other subtle off-by-one errors are present.
- Another bug was uncovered in the _simulateAutocompound function, it is recommended to rethink the approach of writing the simulations and use a more organized and structured approach.
- Avoid overly complicated and overengineered solutions like in the case of the reorderings and replacements of validators, such optimizations are not usually worth it in the long run.
- Fix all the reported issues.
Revision 2.1
We consider all the issues to be fixed correctly. We believe that by fixing the H5 rounding issue no other rounding issues that would cause reverts in main user flows are present. Yet still we recommend fuzzing the protocol to analyze the protocol’s behavior in random scenarios and protocol states.
Ackee Blockchain’s full Everstake audit report with a more detailed description of all findings and recommendations can be found here.
We were delighted to audit Everstake and look forward to working with them again.
