1000 monkeys on 1000 keyboards? (icon ICX bug)
I'm writing/ranting about a recent bug in a smart contract deployed by Icon (ICX) - that has me worried, made me pace frantically and even kept me up at night. But first some background for ppl. Here's the worst bugs and codebase's that I've seen in my career so far.
A Utility application, written in Ruby, where nearly half the live codebase was 'dead code', as in it wasn't used anymore - checked authentication by the "existence" of a header field. This utility was also public facing and didn't need to be. Imagine a Utility application as being an octopus, where each of the arms is working with different internal (and sometimes external) systems and API's. Some of these systems are obviously mission critical. The header field would normally contain a parameter, known in this case as a token. The token is encrypted and can only be created by successful login. In the case of this Utility anybody could set the header fields token to 'You are all fkn idiots', and they would have full access. It was like this for 2 years before I started with them.
1000+ line function that handled creation and set the paramaters of internal financial applications. Whats wrong here is complexity, measured generally by a CRAP index. The more code in a function the more chance of a branch in the code flow that could easily add 0's onto a number - for internal financial tools! And this function was core to the application, obviously monkey patched together. Full rewrite (aka refactor) wasn't in the time schedule, unit testing and decoupling that function was a nightmare but I got through it.
Whole files (in this case once again of javascript) with absolutely no indendation, commenting, and riddled with dead code.
All three of the above were established companies that I contracted with sometime over the past 15 years. If I put more time into it I'm sure I can remember even worst situations. Every time I contract on a new team I make the same argument "we need to aim for 100% coverage" - every time I get laughed at. I even get looked down on as a noob who 'obviously has no experience'. I'm used to it at this stage and stand my ground.... still to this day. Software development is full of fucking idiots that were trained to write spaghetti in educational institutions and then trained to wing it by business ideology leading software iterations/sprints.
Smart contracts are different (light apps that run on blockchain). Massively different. You have no option with smart contract but 100% code coverage 100% of the time. You install or create a 'build server' that uses existing tools to check that there at least exists a test for each function. Code coverage is the percentage of these functions that are tested. Once this is passed then the build server runs the unit tests to test each one. If it fails then the code doesn't get deployed.
The recent bug with Icon should have been tested, it obviously wasn't as it got deployed. This means that no 'Unit Tests' were written, which is extremely dangerous.
Smart contracts:
- are publicly viewable (in the case of public chains), the code of my examples above were internal
- generally move funds/tokens
- are immutable (you can't delete them)
- are deployed by company wallets (private keys of companys smart contract ecosystem)
This also means that because the bug got deployed without any unit tests... there is no proper build server. There isn't even a testnet ecosystem for manual checking. What makes this worse is the type of function that was deployed with the bug. Its known as a 'modifier'. Its basically a function you create and attach to multiple other functions so you don't have to re-write code. So no code reviews either. My conclusion from this with Icon is very harsh, very very harsh...
- no code reviews
- no unit testing
- no build server (or at least functioning one)
- anybody on team has access to use companies 'private keys' to deploy
Anyone of the above would've caught this bug (the 1st and last may be up for debate). We shall see how Icon handle this, as all serious developers will be going WTF.
remember when debating this... we're not talking about JS or Ruby, C#, webdev and API's - we're talking about immutable smart contracts here. 100% code coverage 100% time. Seems Icon might have 1000 monkeys on 1000 typewriters.
https://altcoinbuzz.io/bug-discovered-in-icon-icx-smart-contract/
https://www.cryptoglobe.com/latest/2018/06/smart-contract-bug-disables-800-million-cryptocurrency-icon-icx/