Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

(Multi-sig) Wallet creation within Dapp results in wrong number of owners #444

Open
Sofoca opened this issue Jan 16, 2018 · 1 comment
Open

Comments

@Sofoca
Copy link

Sofoca commented Jan 16, 2018

Expected Behavior

When creating a multi-sig wallet (on Ropsten) with a 2-of-3 signing policy within the Dapp the variable m_numOwners should represent the number of owner accounts (3 in this case).

Current Behavior

m_numOwners is incorrectly set to 4 (instead of 3). After evaluating the contract code I assume that the error originates in a _owners array of the wrong length. My assumption is that the creating account gets added twice (as it also appears twice in some wallets) but I haven't verified this. However adding the wallet in Parity shows the creating account twice under "owners" (don't know if that is any indication).

Screenshot

bildschirmfoto 2018-01-16 um 16 24 36
bildschirmfoto 2018-01-16 um 16 40 06
bildschirmfoto 2018-01-16 um 16 43 05

Steps to Reproduce

  1. Connect Parity or Mist with Ropsten Testnet
  2. Open wallet.ethereum.org
  3. Create multi-sig wallet (e.g. 2-of-3)
  4. Inspect wallet using ropsten.etherscan.io ('read smart contract') or any other tool

A contract that is affected is also deployed at 0x6f6Ae72d72420FF51f7A75c9cd1A1f69e40e1043 on Ropsten.

More Context

I tried to debug some more myself but ran into several problems. E.g. currently two versions of the wallet contract are maintained in this repo and in the dapp-bin repo and it is not immediately clear why and which one is used.

@Sofoca
Copy link
Author

Sofoca commented Jan 25, 2018

Playing with the contract source code gave me some interesting insights. When creating a multi-sig wallet that is supposed to be owned by A, B and C the following results in correct getOwner, isOwner and m_numOwners return values:

  1. Deploying the contract (from A) and
  2. passing the constructor only B and C (not A) as _owners.

This leads me to believe that the standard multi-sig smart contract actually adds the deploying Ethereum account as owner at index 0 and starts adding the _owners array at index 1. As far as I can see this is not properly documented and Ethereum Wallet/Mist implement it incorrectly. Mist actually forbids creating multi-sig wallets where the deploying account is not explicitly named in _owners.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant