Review of SimpleMultisig

SimpleMultisigReview.md

This is an informal... you might even say adhoc review of the SimpleMultisig contract, found here:

https://github.com/christianlundkvist/simple-multisig/tree/9d486cb280c1b0108a64a0e1c4bc0c636919c2d7

This review makes no legally binding guarantees whatsoever. Use at your own risk.

Summary

The two findings listed under Major and Medium should be fixed. The Minor and Note issues don't pose a security risk, but should be fixed to adhere to best practices.

Otherwise, no significant issues were identified. This contract appears to work as advertised, and its simplicity is excellent for the task.

Update: Fixes for my findings have all been merged in.

Specific findings

Major: contract can be "bricked" on deployment if same address is used twice

The contract could be deployed, and instantly unusable. This would occur if the same address is added twice, and the threshold requires all owners to sign in order to execute. The constructor should be modified to prevent this using a similar approach to the execute function.

Resolution: Fixed in coder5876/simple-multisig#5

Medium: Upgrade to solidity ^0.4.14

Prior to 0.4.14, a bug existed in ecrecover.

Ref: https://github.com/ConsenSys/0x_review/blob/final/report/3_general_findings.md#ecrecover-issue-in-solidity-0414

Resolution: Fixed in coder5876/simple-multisig#6

Minor: use require instead of throw

It's easier to read, and throw is being deprecated.

Resolution: Fixed in coder5876/simple-multisig#4

Minor: Indentation of test suite

The utility functions were not properly indented from line 118 to 148 of multisig.js.

Resolution: Fixed in coder5876/simple-multisig#4

Note: Clarify the value of the requirement that signatures be submitted in ascending order

I believe the purpose is to facilitate checking against duplicate signatures, but it would be nice to make that explicit.

Resolution: This has been confirmed by the contract author.