page = a few thoughts on cryptographic engineering – some …
url = https://blog.cryptographyengineering.com
I want to stress that this was an informal code review : I didn’t use any tooling, didn’t even download (most) code to my computer. In fact my “review” mostly involved poking around various Github repositories to see if I could find anything that immediately jumped out as incorrect, and failing that, at least could give me a feeling for the quality of MetaMask’s crypto code. (In fact I did about half the work on my phone while eating a burrito bowl at Chipotle.)
And this last part isn’t just a personal gripe. My TL;DR is that finding and reviewing the correct code was made difficult because there were far too many different organizations owning the dependencies that led to the crypto routines themselves. This made me uncomfortable, given how much money these routines are responsible for securing.
Since many readers have probably never used DeFi, I figure I should give a quick background here on “web3” and MetaMask in particular. If you know something about MetaMask, skip this part.
Ok, so forget everything I did above. That was my mistake. I promise to come back to those newer code paths soon, but that’s for the future. My goal in this review was to look at MetaMask as it exists today. Apparently that means I need to look at Fedor’s elliptic library.
seems poorly defined and serves two slightly different purposes, triggered by a boolean flag — which is just omitted in some calls.
A basic point to make here is that much of this review has come down to reviewing code that solves a simple problem: (deterministically) sampling integers within a precise range. So far I’ve reviewed at least two different custom implementations of the same process, both with slightly different results. Why is the same code repeated so often? Just make this a subroutine so we can analyze it and be sure it’s working correctly.
Anyway the (possible) issues above probably don’t really matter. I’ll assume in most cases private keys are generated correctly using a secure random number generator, which takes the most obvious risks off the table.
If you’re writing a security-critical routine like “sample random integer in a precise range,” write that routine one time , not multiple times. Please!