3 ms·

> 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

> 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 9y ago
The conflation of UI and logic is something I haven't seen since Smarty revolutionized the PHP world.
- bhaak 9y ago
  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 9y ago
    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 9y ago
      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/contracts/snippets/enhanced-wallet.sol https://github.com/paritytech/parity/blob/master/js/src/cont...
starmole 9y ago
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.
- bluejekyll 9y ago
  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.
wocram 9y ago
How can there be solidity "experts" for something that has existed for so little time?

tylersmith 9y ago

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

bhaak 9y ago
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 9y ago
  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 9y ago
    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/contracts/snippets/enhanced-wallet.sol https://github.com/paritytech/parity/blob/master/js/src/cont...

starmole 9y ago

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.

bluejekyll 9y ago
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.

wocram 9y ago

How can there be solidity "experts" for something that has existed for so little time?