Vulnerabilities and Best Practices

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:

  1. 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.
  2. 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:

  1. Uniformity lacked between the repositories, indicative that they were written in different times by different people.
  2. None of the repositories had continuous integration.
  3. No static tools (linters) where found either for the solidity contracts of JS build configurations.
  4. As already hinted above, documentation was sparse and inadequate.
  5. 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:

  1. 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.
  2. 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.
  3. Install code-coverage reports and make them part of the build process.
  4. Require a code coverage of at least 90% and if you want to be whiter than white raise it to 95%+.
  5. Install static analysis tools (linters) for both JS and Solidity codebases.
  6. 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.
  7. 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.

8 Likes

+1
Wow well hopefully in the future more can be done to mitigate this. I’m surprised that the contracts passed security audits. Confident in the team and my investment and the necessity of wrapped staking contracts and the cutting edge products the team is working to create. As a microcap we quite literally can’t afford another mistake. xLending is make it or break it and will require secure, safe, and seemless integrations of xAssets.

I cannot like this post enough. I have similar experience and background, and the week I spend in advance at the start of a new project configuring my CI and unit tests in a massive Solidity project saves me sooooooooo much time down the line.

I find TDD works really well in Solidity where things often may not behave the way you expect with only “trivial” changes, especially once you start getting into storage in proxies and so on.

1 Like

Great post and thanks for putting the time in to write it up. Would love to see all this implemented and the team has indicated as much in the discord so far. Preferably in the future we could also get a blog post prior to each new products’ launch where the team shows this has been implemented. Describe what tests have been ran and how others could easily ran them after following the README, also specifically which team members have reviewed and signed-off on the code.

2 Likes