Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to SimpleMultiSig #147

Closed
wants to merge 1 commit into from

Conversation

elenadimitrova
Copy link
Contributor

@elenadimitrova elenadimitrova commented Feb 2, 2018

Relevant EIPs to MultiSig standards:
ethereum/EIPs#763
ethereum/EIPs#191

SimpleMultiSig release article
https://medium.com/@ChrisLundkvist/exploring-simpler-ethereum-multisig-contracts-b71020c19037

@area
Copy link
Member

area commented Feb 5, 2018

If using this in production, then we can never (or rather should never) require unanimous agreement for the multisig - if someone lost their key with a unanimous agreement required to change to a new multisig, we would be snookered. I also worry about key 'rot' in this scenario - if we don't need the multisig for a long time, then maybe two people would have lost their keys, but not realised it. Even if we could have recovered from one losing the key, we didn't know until two had, and we're snookered again.

This applies to any multisig, it's just that there are fewer 'ways out' with the simpler one if something goes wrong. So long as we adopt it knowing that, I don't have an issue with it.

@jakzilla
Copy link
Contributor

jakzilla commented Feb 5, 2018

What are the ways out available to us if we use the Gnosis one?

@elenadimitrova
Copy link
Contributor Author

With the Gnosis one you still need to get the required number of signatures to change anything, whether it's to add/replace an owner or the required number of signatures. It has additional functionality but not any more ways out than in the SimpleMultiSig.
It has higher max number of owners: 50 as opposed to 10 here which spreads the risk but that is a constant the can be raised if we choose to.

@area
Copy link
Member

area commented Feb 9, 2018

Yeah, Elena is right - I was mistaken in thinking that the threshold for adding/removing signatories was different from that required to do other things. So this applies with either implementation.

@area
Copy link
Member

area commented Feb 9, 2018

Other differences between the two implementations:

  • With simplemultisig, the gas costs are placed on one person (though they are lower in total, I suspect)
  • It will be (I believe) technically more difficult to use. We might need to cook up a (very) simple client.

@elenadimitrova
Copy link
Contributor Author

Some concrete gas costs
SimpleMultiSig only ever needs one transaction (which contains all signatures)

multisig.execute -> 75,413 gas

MultiSigWallet needs an initial transaction submission and then as many confirmations as required, i.e. the confirmTransaction 50,110 gas cost should be multiplied by the required number of approvals -1.

multisig.submitTransaction -> 167,202 gas
multisig.confirmTransaction -> 50,110 gas (not enough signatures gathered)
multisig.confirmTransaction -> 79,857 gas (enough signatures gathered, execute the tx)

@elenadimitrova elenadimitrova force-pushed the maintenance/migrate-multisig branch 3 times, most recently from 0863f0e to f2fc40d Compare February 16, 2018 12:30
@elenadimitrova
Copy link
Contributor Author

After a discussion between @area @jakzilla and myself we decided to shelve this and stay on the Gnosis MultiSig for now at least. Considerations include:

  • gas costs aren't a concern as this will be used for the occasional contract upgrades only
  • using a proven to be (so far) secure contract on the foundation network
    In future I believe EIP 763 will adopt the SimpleMultisig design as a standard which will drive adoption and encourage more work on formal proof and official security reviews. At which point we're likely to reconsider it.

@kronosapiens kronosapiens deleted the maintenance/migrate-multisig branch November 7, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants