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 Customer. 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 cients' 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 Customer with an interim report.
- The Customer 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 Customer 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.
- Verify the fixed code version with all the recommendations and its statuses.
- Provide the Customer with a re-audited 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 funds to be transferred to any party. |
| 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 auditted smart contracts have the logic of voting, based on Aragon standard voting contracts with an additonal objection phase to mitigate the "last-moment" voting problem. After accepting all the "yeas" and "nays" votes, the voting goes to the "objection" phase, when only "no" votes are accepted. The voting power is determined by the voter's balance of a special token (MiniMeToken), allowing to snapshot the voter's balance at the given block, mitigating the risk of the balance's reuse. The voting result is an ability to run the execution script that must be reviewed by voters, its security is not the object of the audit.
| Filename | Description |
|---|---|
Voting.sol |
Contract, allowing a two-phase voting (the voting and objection phases) with MiniMeToken. |
MiniMeToken.sol |
Aragon ERC20 token, allowing to snapshot token balance at the given block number, used for voting. |
| Title | Description |
|---|---|
| Client | Lido |
| Project Name | Two-Phase Voting |
| Timeline | 2022-05-25 - 2022-06-01 |
| Number of Auditors | 5 |
| Date | Commit Hash | Note |
|---|---|---|
| 2022-05-25 | 7e5cd1961697a1bc514bfebdeab08a296e51d700 | Initial audit |
| 2022-06-03 | 32d56a7490702d303be1e09b4e24b767e83751c3 | Reaudit based on fixes provided |
| 2022-06-07 | 4f7646fbc90ac31fada5ed9e95669fc4aecbbc1c | Final commit with all fixes |
The audit covered the following files:
| Filename | Link |
|---|---|
| Voting.sol | https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol |
| Severity | # of Findings |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 1 |
| LOW | 9 |
Smart contracts have been audited and several suspicious places have been detected. During the audit, no critical problems were found, no high, one medium, and nine low issues were identified. After working on the reported findings, all of them were confirmed and fixed by the client.
Final commit identifier with all fixes: 4f7646fbc90ac31fada5ed9e95669fc4aecbbc1c
| File name | Contract deployed on mainnet |
|---|---|
| Voting.sol | https://etherscan.io/address/0x72fb5253AD16307B9E773d2A78CaC58E309d5Ba4#code |
No issues found
No issues found
At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L192 during the objection time, the vote() function checks if the vote is no, if the vote is open for objection and if the voter has balance. But there is no check wether the vote was rejected or not. So it is possible to vote no for a proposal even if it was rejected during the voting time.
It is recommended to add a check if the vote was rejected.
Acknowledged
Acknowledged. The issue is harmless, while the fix will spend extra gas for every vote.
The visibility of the canObject() function at line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L267 which is not called internally from the same contract should be changed to external to save gas.
It is recommended to change public to external.
At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L475
the _isVoteOpenForObjection() function returns true during the main voting time. It can be potentially misleading in functions canObject() and getVote() at the following lines:
- https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L267
- https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L316.
It is recommended to change the _isVoteOpenForObjection() function to return true only during the objection time or to rename the function to better indicate its purpose.
Fixed in https://github.com/lidofinance/aragon-apps/commit/1aa5fc6ec01845485f7a61418bc707a1dccd4965, https://github.com/lidofinance/aragon-apps/commit/c96387aaf5b882faa0969b37f01ac777da1c0de3, https://github.com/lidofinance/aragon-apps/commit/32d56a7490702d303be1e09b4e24b767e83751c3
At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L374-L380 there is a double check of variable _supports.
The assignment at line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L380 can be moved inside the if-else statement to save gas. Additionally, the if-else statement is more readable than a ternary operator.
It is recommended to change to:
if (_supports) {
vote_.yea = vote_.yea.add(voterStake);
vote_.voters[_voter] = VoterState.Yea;
} else {
vote_.nay = vote_.nay.add(voterStake);
vote_.voters[_voter] = VoterState.Nay;
}At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L423 the check is unnecessary since the check at line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L428 returns true if the vote is open during the voting time. The vote cannot be executed until the end of the objection time.
Remove this check or refactor the _isVoteOpen() and _isVoteOpenForObjection() logic.
Fixed in https://github.com/lidofinance/aragon-apps/commit/1aa5fc6ec01845485f7a61418bc707a1dccd4965, https://github.com/lidofinance/aragon-apps/commit/c96387aaf5b882faa0969b37f01ac777da1c0de3, https://github.com/lidofinance/aragon-apps/commit/32d56a7490702d303be1e09b4e24b767e83751c3
At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L433-L440 it is possible to save gas by reading vote_.yea into a memory variable.
For example:
uint256 voteYea = vote_.yea;
uint256 totalVotes = voteYea.add(vote_.nay);
if (!_isValuePct(voteYea, totalVotes, vote_.supportRequiredPct)) {
return false;
}
// Has min quorum?
if (!_isValuePct(voteYea, vote_.votingPower, vote_.minAcceptQuorumPct)) {
return false;
}At line https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L368 a voter can change their vote.
The malicious voter can initially vote yes misleading other users about their intention, then change their vote to no at the last moment potentially blocking the proposal.
It is recommended to check to prevent the last-minute no attacks against proposals.
Acknowledged
It's intentional, because we don't want to lose a possibility to convince voters to change their decision if some disastrous outcome is possible. In exchange we got this possibility for malicious voter to unexpectedly block the voting, but the possible harm is limited in this case.Also, we want DAO voters to be aware of these caveats too, so we are going to add a paragraph about to https://lido.fi/governance
A voting passes two stages:
- Vote is open for yeas and nays
- Vote is open for nays only
To check for those stages these methods are used:
_isVoteOpen,_canVote,canVote_isVoteOpenForObjection,_canObject,canObject
Namings to check that the voting is in the first stage are confusing, because name _isVoteOpen doesn't clearly say that it is checking the first stage only and for the second stage of voting it will return false. The same thing refers to canVote() and _canVote(). Their names suggest that they should return true for any unfinished voting, while they return true only if the voting is in the first stage.
This confusion may lead to bugs in the future if the contract code base becomes larger and it will become harder to keep in mind that the behavior of some methods differs from what their name seems to imply.
There are already cases of the incorrect use of methods in the code: https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L384
function _vote(uint256 _voteId, bool _supports, address _voter) internal {
...
if (!_isVoteOpen(vote_)) { // objection phase
...
emit CastObjection(_voteId, _voter, voterStake);
}It is obvious that instead of !_isVoteOpen(vote_) you should use _isVoteOpenForObjection(vote_). It is not a vulnerability now because in the current context these two lines are interchangeble.
However, if in the future the development team decides to add another voting phase, they will have to find and fix all the lines with the incorrect use of methods and if the code base becomes large and the namings would be still confusing it will be easy to miss some line and create a vulnerability.
Change method names to be more clear about what they really do. For example, instead of _isVoteOpen you could use _isVoteOpenFirstStage. And instead of _canVote you could use _canVoteFirstStage.
The canVote method is a public method so it may be better to keep its name for API compatibility. But perhabs it should be marked as deprecated in the code comments and another,canVoteFirstStage method should be added.
Fixed in https://github.com/lidofinance/aragon-apps/commit/1aa5fc6ec01845485f7a61418bc707a1dccd4965, https://github.com/lidofinance/aragon-apps/commit/c96387aaf5b882faa0969b37f01ac777da1c0de3, https://github.com/lidofinance/aragon-apps/commit/32d56a7490702d303be1e09b4e24b767e83751c3, https://github.com/lidofinance/aragon-apps/commit/4f7646fbc90ac31fada5ed9e95669fc4aecbbc1c
https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L234 https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L247 https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L257 https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L267 https://github.com/lidofinance/aragon-apps/blob/7e5cd1961697a1bc514bfebdeab08a296e51d700/apps/voting/contracts/Voting.sol#L324
We recommend adding descriptions for the mentioned public methods parameters.
The problem with the "last-minute" vote in this scheme is not fully mitigated, because a possibility of a vote to be denied at the last moment still persists. An attacker can object and deny the voting at the last moment like it could be done with the previous version of voting.
Should be simply acknowledged by the Lido team.
Acknowledged
It's a compromise that we're aware of. From our point of view, the possibility of passing the untelegraphed changes through the DAO is much more risky than last-minute blocking, whose possible impact is limited. Also, we want DAO voters to be aware of these caveats too, so we are going to add a paragraph about to https://lido.fi/governance
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.