Making Hermez SAFU — Secureum #4

Security Audits For Users

The previous post hypothesized that we can take a step towards making DeFi “SAFU” by breaking down Security Audits For Users (SAFU) and illustrated that with a case study of a DeFi stablecoin project.

This post dives into another interesting DeFi project — Hermez payment network, an Ethereum Layer 2 (L2) Zero-Knowledge (Zk) Rollup solution. 

For some context from the previous post, DeFi projects rely on external security audits as a stamp of security approval. Audit is not a security warranty of “bug-free” code by any stretch of imagination but a best-effort endeavour by trained security experts operating within reasonable constraints of time, understanding, expertise and of course, decidability.

Audit reports illustrate security issues with descriptions of underlying vulnerabilities, severity/difficulty, potential exploit scenarios and recommended/resolved fixes. They also provide subjective insights into code quality, documentation and testing. The scope/depth/format of audit reports varies across auditing teams but they generally cover similar aspects.

Security companies execute audits for clients who pay for their services. Engagements are therefore geared towards priorities of project owners and not project users. Audits are not intended to alert potential project users of any inherent risk. That is not their business/technical goal. 

Nevertheless, the conjecture is that project users may be basing their risk without considering if the project had any security audits or incorrectly assuming no/minimal risk for audited projects without bothering to, or having the expertise (understandably so) to evaluate audit report contents. 

Evaluating audit reports requires a reasonable level of expertise in smart contract security. Breaking down security audit reports for the benefit of less security-savvy users may make DeFi a bit safer by helping them DYOR.

This post gives a breakdown of audit reports on Hermez project from Solidified and Trail of Bits (ToB), and draws attention to key report highlights.

Context

Project Overview: Hermez is a decentralized Zk-Rollup focused on scaling payments and token transfers on top of Ethereum. 

It is a L2 Zk-Rollup scalability solution from Iden3 to help with their mission of privacy-preserving self-sovereign identity on public blockchains. It uses SNARKS (Iden3’s Circom and SnarkJS libraries) with validity proofs and on-chain data to scale to 2000 TPS on Ethereum.

Actors called “Coordinators” collect L2 transactions into batches, compute validity proofs, and post them with the transaction data onto Ethereum L1. Coordinator candidates bid in an auction for Coordinator rights during a slot which is 40 Ethereum blocks or ~10 minutes.

The winning candidate rolls up its batch of L2 transactions to L1 and is rewarded with 30% of bid proceeds. The remaining 30% is burned and 40% is donated to Gitcoin quadratic funding pool (hence called Proof-of-Donation) for funding other Ethereum ecosystem projects. Auctions and rewards use the project’s HEZ token. More details may be found in their documentation.

There have been two security audits so far. Hermez has recently launched a bug bounty program.

Audits Overview: The first audit was with Solidified and a second one with ToB immediately after that.

The Solidified audit was performed in parallel by three auditors (duration not mentioned) during October 2020 reporting no critical/major issues, 3 minor issues and 5 informational notes. All issues were acknowledged and team responses captured for 2 of the minor issues. The expectation was that all reported issues are fixed whereas notes (referring to best practices or optimizations) are up to the team’s discretion.

The ToB audit was a four person-weeks effort with three auditors completing it in two weeks from October 26 to November 9, 2020. They reported 22 issues of which 6 (7 seems to be an error; one of them is in fact low-severity) were high-severity and 4 medium-severity. Rest were of low/informational severity. The overall assessment was that contracts follow Solidity best practices and avoid common security pitfalls but can improve the specification and identify/verify corner cases using fuzzing or symbolic execution. Since the audit, the project made some of the recommended fixes which were further reviewed by ToB. (The audit also covered their Circom circuits used for zero-knowledge proofs but this post focuses only on their smart contracts.)

Solidified Report Breakdown

This audit reported no critical/major issues, 3 minor issues and 5 informational notes (all acknowledged by the project) as shown below. 

The first issue points to the risk of a Hermez-controlled governance process on token values/withdrawals because of variance in prices over time. The project response indicates that this governance centralization is part of a guarded-launch and is planned to be removed after gaining confidence in proper system functioning.

The second issue is about a missing zero address validation in setDonationAddress() function of HermezAuctionProtocol.sol, which seems to have been fixed since then.

The third issue was related to tokens that charge a transfer fee in their protocol (e.g. DGX) which will cause an accounting mismatch between L1 and L2 layers of Hermez. Such tokens are presumably rare and project’s response was to simply not support such tokens.

The note on malleable signatures appears fixed. The note on coordinatorURL endpoint validity check also appears fixed. The note on roles and zero addresses seems still relevant although the role names and logic seem to have changed in their governance update. Note 7 is an optimization and Note 8 on delayed withdrawals will be addressed by alerting users.

Overall, no red flags in the report and most issues/notes are addressed.

ToB Report Breakdown

The audit report is considerably elaborate.

Executive Summary: This concludes with a high-level recommendation that says:

The mention of contracts following Solidity best practices and avoiding common security pitfalls is reassuring. The note on improving specification and identifying/verifying corner cases using security tools before audits is worth some consideration because it lets auditors focus on deeper issues. 

Project Dashboard:The project dashboard gives an excellent overview of the assessment:

A total of 22 vulnerabilities with 6 high-severity and 4 medium-severity ones discovered within four person-weeks effort is concerning but needs to be evaluated in the context of their difficulty of exploitation, project’s responses and post-audit fixes.

Furthermore, note the 11 Data Validation, 2 Access Controls and 3 Undefined Behavior vulnerabilities. Data validation vulnerabilities result from missing sanity checks on input data that may be attacker controlled and therefore necessitate greater attention. Access control to privileged operations need extra validation because they could subvert the functioning of the entire protocol via unauthorised/unexpected changes to critical parameters or token dynamics. Similarly, undefined behavior means that triggering those vulnerabilities could result in potentially malicious unexpected results and hence should not be dismissed. Some other categories such as  Auditing & Logging or Patching may relatively not be as critical.

Code Maturity: The report then presents a qualitative code maturity assessment across eleven classes and rating criteria:

Note the 8 classes with Moderate rating (Appendix B: The component had some issues) and 2 classes with Satisfactory rating (The component had only minor issues), which seems a bit concerning. The ratings are justified by corresponding vulnerabilities discovered. Key Management class wasn’t investigated.

Absence of overflow/underflow checks (i.e. SafeMath), centralized governance (although this seems to have been updated as mentioned earlier), reliance on delegatecall proxy pattern causing vulnerability to front-running, undocumented incident response plan, missing function-level specifications, absence of extensive testcases and the lack of apparent use of static analysis tools or fuzzers are the flags raised here.

But it is good to see that there are no Weak (The component led to multiple issues; more issues might be present) or Missing (The component was missing) ratings.

Coverage: The engagement covered the contracts responsible for managing the L1/L2 interactions with coordinators (Hermez.sol), auction (HermezAuctionProtocol.sol) and the withdrawal logic (InstantWithdrawManager.sol and WithdrawalDelayer.sol). It also covered the  Circom circuits used in zero-knowledge proofs (out of scope for this post).

It appears that post-audit, Hermez has added a new governance contract HermezGovernance.sol which wasn’t covered in audits.

Verification/Analysis Tools: Fuzzing tool Echidna doesn’t appear to have been used for any of the findings. Symbolic Execution tool Manticore has been suggested for detecting TOB-HERMEZ-015, TOB-HERMEZ-016, TOB-HERMEZ-018, TOB-HERMEZ-019 and TOB-HERMEX-021. Static analysis tool Slither was used to detect TOB-HERMEZ-009, TOB-HERMEZ-010, TOB-HERMEZ-013 and TOB-HERMEZ-022.

Recommendations: A list of short-term and longer-term recommendations are presented, some of which are to consider delegatecall proxy pattern pitfalls, low-level Solidity risks, blockchain-based voting drawbacks, use of automated tools like Slither/Manticore, external dependency risks and more documentation/logging.

Findings Summary & Details: Details are presented for all reported vulnerabilities highlighting their type/severity/difficulty, code segments, vulnerability descriptions, exploit scenarios and recommendations. 

Of the 6 high-severity issues, 5 are high-difficulty, 1 is medium-difficulty and none are low-difficulty (see Appendix A for definitions of these difficulty levels).

Fix Log: This (Appendix F) indicates that 13 of the 22 reported vulnerabilities have been fixed. 8 were not fixed and 1 is partially fixed. The details in this section describe the project team’s responses for the findings and those not fixed. 

The transaction and account spam issues (TOB-HERMEZ-003 & TOB-HERMEZ-004) were deemed as non-issues by the project team given the costs and design. TOB-HERMEZ-007 wasn’t fixed because that functionality is specific only to upgrading and might impact design of already audited external components. TOB-HERMEZ-009, which deals with missing zero address checks, wasn’t completely addressed because these were addresses meant to set to 0 in future per governance requirements (also, this governance code has been updated since the audit as mentioned earlier). 

TOB-HERMEZ-012, a high-severity front-running issue due to delegatecall proxy pattern, was partially addressed by adding checks in deployment script to detect any such events. TOB-HERMEZ-013, another high-severity reentrancy issue in TokenHez was deemed as a non-issue by project team because it is a pre-existing non-ERC-777 token which does not have reentrancy risks. TOB-HERMEZ-014, a medium-severity issue relating to the size of chainId variable, was not fixed but the documentation on the size assumption was updated. TOB-HERMEZ-019, an informational issue, was not fixed for the same reason as TOB-HERMEZ-007 above. TOB-HERMEZ-020, a low-severity issue related to third-party dependencies, also wasn’t fixed with the response that dependencies will be frozen and checked before deployment.

The rationale for not fixing the above issues seems mostly reasonable at a glance but could indicate some residual risk in the project. Rest of the issues were fixed and further reviewed by ToB, which is reassuring.

Conclusion

This post gave a breakdown of audit reports on Hermez project — a network for scaling payments and token transfers on Ethereum using Zk-Rollups — from Solidified and Trail of Bits (ToB). Overall, there were some red flags raised which were addressed by fixes or project’s justification for status quo.

Furthermore, the project seems to have made some updates to governance (and other contracts) post-ToB-audit and it appears that these changes were not evaluated by ToB during their fixes review. This highlights another challenge of audits: project upgrades after audit may impact security without the benefit of continued external assessment because of logistical/financial reasons. Projects need in-house smart contract security expertise.

Assessments from two audit teams may help get different perspectives and catch issues missed by either one. It may help to fix issues from the first before scheduling the second. The second audit may also verify the issues/fixes from the first one without getting biased. There are pros/cons to enable knowledge transfer between the two audits or to keep them independent.

Breaking down DeFi Security Audits For Users (SAFU) may be worthwhile as a public good but certainly should not be interpreted as an endorsement/indictment of projects/auditors or as financial/investment advice. DeFi SAFU should be a collective community responsibility.

I hope you found this somewhat useful. Thanks for reading and look forward to your comments and feedback.