So this just happened. It’s late, but before heading to bed I wanted to quickly write-up a technical analysis of this one because it’s quite short.
One of the quickest ways to understand a vulnerability is to look at its patch if one is available. Let’s do just that. There are actually a couple of now closed pull requests related to the fix, but the very first one tells us the story behind this vulnerability. The diff can be found in Parity’s GitHub repository here.
For readers who aren’t familiar with the Solidity language, the added keywords at the end of the first two differing functions are visibility specifiers. Visibility specifiers dictate who is allowed to call specific functions, just as they do in other programming languages. Sometimes functions simply aren’t fit for public use, either because of security reasons or API design.
What’s does the internal visibility do that got added in the pull request? Consulting the Solidity docs, we find:
internal: Those functions and state variables can only be accessed internally (i.e. from within the current contract or contracts deriving from it), without using this.
So the crux of this vulnerability is that several privileged contract functions were left public.
Another detail is that calls to the main contract were delegated to the vulnerable contract which acted as a library, making this issue a little bit harder to see. I would argue not by much though, especially considering that the main contract simply takes incoming calls and delegatecalls to the vulnerable library contract. The design is hard to miss.
The result was that anybody that knew the address of a vulnerable contract could call these functions and change the configurations of these contracts, including the list of contract owner addresses.
You may be wondering how these privileged functions were made public in the first place. The answer is actually in a lack of code. You see, unless otherwise specified, the visibility of a Solidity function is public.
When I started learning Solidity and came across this detail, I was surprised. Contract developers should be explicit about what functions are allowed to be called externally. This is akin to writing an API and having to explicitly set every function of your application to be private, unless you want them exposed to the Internet.
Needless to say, I think that this is a bad convention for a smart contract programming language.