The Multi-Sig Hack: A Postmortem
blog.parity.io
blog.parity.io
Except for the design issues with the Solidity language that have been discussed on here before, this massive process failure seems to have been a main factor allowing this to happen.
- Security critical changes needed extra tagging for more in-depth review. The other way around seems a safer default to me: By default require additional security review, and only under certain conditions allow for a lighter review.
- In the basic review apparently they didn't check for correct tagging and failed to notice that a UI change was also changing contract code without being tagged as security-relevant.
- It seems to be a terrible idea to mix in UI changes with contract code changes. Feels to me that such a change should be simply rejected in review, and re-reviewed after splitting out the changes.
The bug wasn't that the UI and business logic were (or are, I haven't looked at the code) intermingled.
But that a refactoring of the UI and a refactoring of the contract code were done in the same step.
If they were done separately, the smart contract refactoring wouldn't have slipped through unscrutinized.
[0]: https://github.com/paritytech/parity/blob/master/js/src/cont...
I know of very few organizations that only rely on CLI for reviews.
Have the owners of those exploited wallets come forward yet?
I am wondering how long it will be before a wallet developer is targeted with a civil lawsuit. In particular, looking at Parity's web site, I am seeing the noticeable size difference between this text…
> Parity is the fastest and most secure way of interacting with the Ethereum network.
(Front and center on https://parity.io/index.html)
… and this text:
>We accept no liability for your use of the software or its source code (save to the extent such liability cannot be excluded as a matter of law).
(From https://parity.io/parity.html, at the bottom of the "Sample Parity" section.)
Mostly all the accounts affected were of ICOs with huge funds.
The 3 particular wallets belonged to:
Swarm City (https://swarm.city) Aeternity (https://www.aeternity.com) Edgeless Casino (https://edgeless.io)
It is exactly zero lines of code. ;-)
On a more serious note, I am not sure why anyone would make that claim. As noted in an above post, they absolve themselves of legal liability, but they do appear to be making a claim of fitness for purpose.
Why start from scratch and not use something like Haskell or ML? I don't understand why they're reinventing the wheel when there's a chance that millions could be stolen..
The bug was in the contract language, https://solidity.readthedocs.io/en/develop/
I was going to dismiss this on the grounds that it is hard to compete with the implicit bounty that all the funds in contracts present to the black-hats, but on second thoughts, it would have the benefit of motivating ethical hackers who are too skeptical of Solidity to have anything at risk in Ethereum. It seems plausible that skeptical people are more likely to find bugs, all else (including motivation) being equal.
Note, I work for Parity.
I hope I'm not the only one finding this concerning? Companies that work on trivial products (in the sense that mistakes are not irreversible loss of other peoples money) often do more than one peer review.
https://news.ycombinator.com/item?id=14807779
> The restructuring of the original multi-sig wallet contract happened as part of a much larger change, namely the introduction of the Wallet user-interface, a 4000-line, almost entirely Javascript, CSS and HTML alteration.
The pull request in question[0] adds 2228 lines and removes 918. Not sure how this adds up to 4000 lines. But let's look at those lines:
* 685 were in a related "wallet.sol" file, though most of it due to changing indentation from 2 to 4 spaces. If you revert that, the diff goes down to 79 lines.
* 793 were in the buggy contract, "enhanced_wallet.sol". Same thing applies here, 520 lines were due to the change in indentation spaces.
* 477 + 466 lines are what appears to be auto-generated JSON related to the above.
Summing it up, out of the claimed 4000 lines:
* 854 were pure exaggeration,
* 1126 were whitespace changes,
* 943 were autogenerated files related to the contracts,
* 79 were in a related contract,
* 273 were in the buggy contract,
* 725 were other changes,
* 0 were HTML or CSS changes.
Perhaps the "alteration" referred to also includes related changes not part of the same pull request, though I was not able to find any related pull requests close in time to this one. But since the article refers to a 4000-line "alteration" which supposedly consists mainly of HTML, CSS and Javascript, I sure hope there is some other changeset referred to, and that this is not just a blatant lie.
> The first and biggest change will be to ensure that any alterations to the codebase that involve live contract code (which can be generally identified through .sol files) be reviewed by Solidity experts. At present the multi-sig wallet is the only Solidity code that is user-deployable and in wide use within Parity.
A few paragraphs up, you say that you already had exactly that policy:
> Changes which are understood to be sensitive (such as with contracts or core consensus code) are tagged as such and undergo a greater level of review, generally two or more expert reviews.
At no point does Parity seem to take responsibility for what happened, and the article ends with a paragraph trying to shift blame to the design of the Solidity language.