After the second hack I quickly skimmed through the open source repositories of xToken, in particular, I reviewed the xsnx, xknc, xaave and xu3lp contracts.
This is not a thorough DD, I time-boxed this to a 30 minute review and 30’ to write this post. In case you are not familiar with me, I’ve been a software engineer for over 20 years, the past 10 years I’ve been leading engineering teams and building organizations’ operations and applying best practices under the requirements of companies for whom security was an existential threat. I’ve lead Information Security Management Systems implementations and got and held ISO27001 certifications for engineering organizations. tl;dr, I know what I am talking about.
I reviewed xsnx in regards to the particular second exploit, as explained by the post mortem. I will sum up my findings in two observations:
- The offending code was created as a response to the first hack that occurred in May. A time when security awareness was at the highest degree at the xToken organization.
- The offending code was not, and remained untested.
I tried to run the tests of the xsnx contract on my local, following the README instructions, but was not able to. The contracts compiled, env variables set, but all tests broke due to a variable not set or a broken configuration. I don’t doubt that the project can be built and tested, what I am saying is that the instructions in the README where inadequate to perform that task.
Further skimming through the repositories and contracts, I observed that xknc and xaave didn’t even have README files and all attempts to build and test the projects on local failed as well.
xu3lp had a README, but the build and test results were similar to xsnx.
A couple more observations across all the repositories:
- Uniformity lacked between the repositories, indicative that they were written in different times by different people.
- None of the repositories had continuous integration.
- No static tools (linters) where found either for the solidity contracts of JS build configurations.
- As already hinted above, documentation was sparse and inadequate.
- None of the projects could be built out of the box and without assistance from the developers / authors.
My recommendations for moving forward, following best practices and being security aware:
- Document all projects so they can be built and tested on local machines without any assistance other than consulting with the README. This will enable scaling of the team and more eyes and consequently, scrutiny to fall on the projects.
- Build all projects on continuous integration services (circleci is free). This will raise trust and confidence in the codebase and will also help new developers understand how the project can be build in a practical manner.
- Install code-coverage reports and make them part of the build process.
- Require a code coverage of at least 90% and if you want to be whiter than white raise it to 95%+.
- Install static analysis tools (linters) for both JS and Solidity codebases.
- More detailed and explicit package.json, leverage other OSS utils like .nvmrc, .editorconfig, .prettierrc to drive uniformity, be explicit with node version the project builds against and generally raise code quality and security.
- Instate firm process in regards to code reviewing. Code reviewers share responsibility of the delivered code and should be an unbreakable protocol that all PRs go through at least 2 reviewers who have to sign off.
If you are an end-user and have to remember one thing from everything I described above, keep this: tests will save your ass.
Demand from DeFi projects that they display front and center their testing coverage and do not compromise for anything less than 90% test coverage.
90% is the golden standard for the web industry. I am being too lenient here as money is involved. I’d personally not allow anything less than 100% test coverage. Yes it costs and takes more time, but ensures that errors will not happen and money will not be lost. And since money is involved, a happy-path way of testing is barely adequate. All sorts of testing have to be employed in order to catch unforeseen conditions.
Security is not a good or nice to have. Even more so in the DeFi space where money is involved. Also security, is not a nebulous, undefined thing floating over there. It’s a very specific, concrete and well defined concept, with rules, processes and best practices.
This hack could be avoided should best practices were applied and followed.