The Multi-Sig Hack: A Postmortem(blog.parity.io)
blog.parity.io
The Multi-Sig Hack: A Postmortem
https://blog.parity.io/the-multi-sig-hack-a-postmortem/
26 comments
The conflation of UI and logic is something I haven't seen since Smarty revolutionized the PHP world.
That seems unnecessary snarky to me.
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.
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.
It sounds like the code which should be maintained separately, is being maintained in the same repo. This type of issue would be easier to catch with different policies in different repos.
If not in a separate repo, then at least properly namespaced away from unrelated code. But no: the contract in question lives in the "Javascript API" subproject, on the path "js/src/contracts/snippets/enhanced-wallet.sol"[0].
[0]: https://github.com/paritytech/parity/blob/master/js/src/cont...
[0]: https://github.com/paritytech/parity/blob/master/js/src/cont...
How can there be solidity "experts" for something that has existed for so little time?
This is interesting. How much of this is to blame on GIT? Getting used to big changes and no good graphical diff by default? In GIT command line it is easy to accept, but quite cumbersome to reject. So auditors tend to just trust by person, and not by code. Which might work great for the kernel, where there is a strong trust, but for things like this it might blow up.
Do you know that they don't use something graphical? Like GitHub or sepmthing similar for code reviews?
I know of very few organizations that only rely on CLI for reviews.
I know of very few organizations that only rely on CLI for reviews.
>… 3 multi-sig wallets were exploited from of a total of 596 vulnerable multi-sig wallets …
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.)
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.)
> Have the owners of those exploited wallets come forward yet?
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)
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)
yeah claiming to be the the fastest or most secure anything is almost always bullshit
I wrote the most secure, and fastest, operating system.
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.
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.
> Parity is the fastest and most secure way of interacting with the Ethereum network.
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..
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..
Is Haskell or ML 100% secure and not prone to bugs? The original multi-sig wallet code did not contain the error. It was only when Parity refactored it did they introduce the error. Sure, it would have been nice had Solidity not defaulted functions to public, but the error was the fault of a human and not the language. That said, I agree with their recommendations that functions not default to public and the use of simple contracts encapsulating complex contracts.
No, not inherently more secure, but probably more developers around who already understand the core semantics. Also, tools.
The core is written in Rust which is at least as strong as ML.
The bug was in the contract language, https://solidity.readthedocs.io/en/develop/
The bug was in the contract language, https://solidity.readthedocs.io/en/develop/
Strictly speaking, the bug was not in the language itself, but in a specific contract's code.
Looks like it needs some work, anyway:
https://news.ycombinator.com/item?id=14810008
https://news.ycombinator.com/item?id=14810008
The language and the underlying EVM code worked exactly as intended. The bug was in the contract written by Parity.
"Going forward, Parity will try to arrange a bug-bounty programme."
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.
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.
Instead, or before, creating a security bounty pay well known security researchers and you will obtain solid results. There is no enough good security people in the world to give attention to so many underpaid bounties. If there are hundreds of millions at stake, couldn't you pay <=1% as a safety measure?
They claim to be the most secure wallet, and at the same time allow code without any review to get submitted. I feel that the whole Ethereum space is full of lies like this (I stuck with Bitcoin core, as I feel that the team is much more cautious).
The code was reviewed by two other developers and this is the standard procedure for any pull request touching any of our code-base. The mistake that happened here, that security-relevant changes were not highlighted accordingly and should have split out in a different pull request which requires more eyes (sample: https://github.com/paritytech/contracts/pull/48).
Note, I work for Parity.
Note, I work for Parity.
"code changes undergo at least one peer review which we manage through a largely manual system of tracking and tagging, commonly found in open source projects."
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.
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.
previous discussions about the vulnerability:
https://news.ycombinator.com/item?id=14807779
https://news.ycombinator.com/item?id=14817557
https://news.ycombinator.com/item?id=14825437
https://news.ycombinator.com/item?id=14807779
https://news.ycombinator.com/item?id=14817557
https://news.ycombinator.com/item?id=14825437
This postmortem is disingenious at best and lying at worst.
> 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.
[0]: https://github.com/paritytech/parity/pull/4805
> 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.
[0]: https://github.com/paritytech/parity/pull/4805
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.