The Grin project has completed its second security audit, this time conducted by Coinspect. You can find their blog on the audit here and the full report with a table tracking each issue found can be accessed in the new grin-security repo.
During this time, in addition to addressing the issues discovered, a new RFC was implemented to further improve Grin’s security processes to improve response processes and timeliness among other things going forward. It is important to continue to evaluate Grin’s security by improving processes and conducting ongoing security audits as the community and codebase change over time.
Findings Summary
One critical vulnerability was found which was promptly fixed and disclosed with CVE-2019-9195. Additionally five high risk, seven medium risk and one low risk issues were reported. All findings have now been verified to be fixed by Coinspect.
Most of the issues related to:
Directory traversal and file handling
Unsafe code in third-party libraries
Improperly handled errors in Rust
P2P connection logic
Insufficient validation
These types of issues can lead to denials of service, data corruption and privilege escalation. Coinspect additionally identified some high-level issues including third-party dependencies, transaction pool/eviction policy, transaction processing time and transaction creation workflow to be future goals to improve Grin’s security. More details about these suggestions can be found in the full report.
Conclusion
All identified issues have been fixed. There are still some areas highlighted above and in the full report that can still be improved on as future work for Grin development.
Coinspect produced effective work with their findings and reporting, especially considering the challenges of coordinating this type of service with a decentralized community. Additionally the audit would not have been possible without the amazing donors that helped Grin reach the audit funding goal last December, the developers that followed through on important but tedious work and everyone else who gave their time and energy to give us a more secure Grin.
Hopefully the community that contributed to making this possible feels that this was a good use of resources and time, can share and iterate feedback from the process to make improvements and ultimately integrate what we have learned into future audits to continue the never-ending process of making Grin as secure as possible.
This looks pretty awesome! For PR#2624 in relation to Grin-001, were there any protocol bumps or anything along those lines to enforce that users are operating on a secure version? Also, what was the reasoning behind having Grin-0013 be Medium versus High? Could this vulnerability have been used to stall block production by spamming orphans into the pool? All in all fantastic job by Coinspect, extremely solid work.
For PR#2624 in relation to Grin-001, were there any protocol bumps or anything along those lines to enforce that users are operating on a secure version?
I don’t think so. The CVE and upgrade instructions were relayed to the community but I don’t think anything was hardcoded at the time to make sure users upgraded (the hardfork ensured users upgraded, and someone may come correct me about version enforcement directly related to the CVE/Grin-001).
Also, what was the reasoning behind having Grin-0013 be Medium versus High? Could this vulnerability have been used to stall block production by spamming orphans into the pool?
Coinspect set the ratings but I’ll speculate. While it would be cheap to spam orphan blocks if they are not properly validated to cause the DOS scenario, it doesn’t have a high impact. This only impacts the OrphanBlockPool data structure, which keeps track of orphan blocks (when the maximum size is reached, too old/too far ahead orphan blocks are evicted).
This attack would clog up the OrphanBlockPool which would increase bandwidth consumption and potentially cause legitimate orphan blocks to be evicted. This is wasteful and it may add latency to block propagation but ultimately it only impacts the OrphanBlockPool- it would not prevent block production. The attack would have a more severe impact on lower resource nodes with potential crashes as it could have grown the orphan pool >1Gb.
tl;dr It would have only impacted the orphan pool and would not have prevented block production (block propagation latency however could be impacted in some cases- it could be argued that this could impact block production, but in a less direct way). Very low resource nodes may have crashed.
Great explanation, I guess I misread that part of the explanation in the report.
That’s good to hear! I was actually trying to find where in code the protocol version was set and was having trouble finding it
I would be super interested to learn about Coinspect’s methodology, was the majority of the code-audit done manually? Were they able to use any static code analysis software, and if so, was it specifically tooled to work with cryptocurrencies?
