
The Multi-Sig Hack: A Postmortem - ca98am79
https://blog.parity.io/the-multi-sig-hack-a-postmortem/
======
anonymouz
> 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.

~~~
tylersmith
The conflation of UI and logic is something I haven't seen since Smarty
revolutionized the PHP world.

~~~
bhaak
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.

~~~
bluejekyll
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.

~~~
nemetroid
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...](https://github.com/paritytech/parity/blob/master/js/src/contracts/snippets/enhanced-
wallet.sol)

------
CaliforniaKarl
>… 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](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](https://parity.io/parity.html), at the
bottom of the "Sample Parity" section.)

~~~
zengid
> 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..

~~~
kev009
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/](https://solidity.readthedocs.io/en/develop/)

~~~
tsukaisute
Strictly speaking, the bug was not in the language itself, but in a specific
contract's code.

~~~
zengid
Looks like it needs some work, anyway:

[https://news.ycombinator.com/item?id=14810008](https://news.ycombinator.com/item?id=14810008)

------
mannykannot
"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.

~~~
wslh
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?

------
xiphias
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).

~~~
5chdn
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](https://github.com/paritytech/contracts/pull/48)).

Note, I work for Parity.

------
redm
"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.

------
kanzure
previous discussions about the vulnerability:

[https://news.ycombinator.com/item?id=14807779](https://news.ycombinator.com/item?id=14807779)

[https://news.ycombinator.com/item?id=14817557](https://news.ycombinator.com/item?id=14817557)

[https://news.ycombinator.com/item?id=14825437](https://news.ycombinator.com/item?id=14825437)

------
nemetroid
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](https://github.com/paritytech/parity/pull/4805)

