> 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 depth and nature of changes made (and thus the severity of the potential bug) was misunderstood by the rest of the team. It was erroneously tagged as a (purely) UI change and thus received only one review before merge. A later audit by a Solidity expert also missed the issue.
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.
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].
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.
>… 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.
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.
> 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..
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.
"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.
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).
"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.
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.
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.