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

Fix initialisation bug. #6102

Merged
merged 1 commit into from Jul 19, 2017

Conversation

Projects
None yet
@gavofyork
Member

gavofyork commented Jul 19, 2017

No description provided.

@gavofyork gavofyork merged commit b640df8 into master Jul 19, 2017

2 checks passed

continuous-integration/gitlab-js-test Build stage: test; status: success
Details
continuous-integration/gitlab-test-rust-stable Build stage: test; status: success
Details
@SCBuergel

This comment has been minimized.

Maybe assert(m_numOwners == 0) instead of if (m_numOwners > 0) throw; _;?

This comment has been minimized.

ajayatgit replied Jul 19, 2017

No, it is a correct modifier. If you see the usage
image

function initwallet should be only called if the wallet initialization has not happened,so it should throw for all cases if m_numOwners > 0

This comment has been minimized.

devedse replied Jul 19, 2017

The latest sol compiler shows the following warning when doing a throw:
Warning: "throw" is deprecated in favour of "revert()", "require()" and "assert()"

So indeed the modifier is correct, except the 'throw' statement has been deprecated.
See SolidityDocs Exceptions for more information.

This comment has been minimized.

ccding replied Jul 25, 2017

Actually we should use require().

@admazzola

This comment has been minimized.

admazzola commented on e06a1e8 Jul 19, 2017

Who is auditing this code that ends up affecting $100 millions worth of currency ? :/

This comment has been minimized.

3esmit replied Jul 19, 2017

Why save addresses as uint? This contracts seems to be overcomplicated to what it wants to do. See Consensys Multisig, they have a much simplier approch.

This comment has been minimized.

jomo replied Jul 20, 2017

@admazzola This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.

THERE IS NO WARRANTY FOR THE PROGRAM, TO THE EXTENT PERMITTED BY APPLICABLE LAW. EXCEPT WHEN OTHERWISE STATED IN WRITING THE COPYRIGHT HOLDERS AND/OR OTHER PARTIES PROVIDE THE PROGRAM "AS IS" WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE OF THE PROGRAM IS WITH YOU. SHOULD THE PROGRAM PROVE DEFECTIVE, YOU ASSUME THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.

IN NO EVENT UNLESS REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING WILL ANY COPYRIGHT HOLDER, OR ANY OTHER PARTY WHO MODIFIES AND/OR CONVEYS THE PROGRAM AS PERMITTED ABOVE, BE LIABLE TO YOU FOR DAMAGES, INCLUDING ANY GENERAL, SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES ARISING OUT OF THE USE OR INABILITY TO USE THE PROGRAM (INCLUDING BUT NOT LIMITED TO LOSS OF DATA OR DATA BEING RENDERED INACCURATE OR LOSSES SUSTAINED BY YOU OR THIRD PARTIES OR A FAILURE OF THE PROGRAM TO OPERATE WITH ANY OTHER PROGRAMS), EVEN IF SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.

This comment has been minimized.

benjyz replied Jul 20, 2017

@jomo license needs fixing. if there is no "warranty" or any promise of security at all, then use of this software should be actively discouraged. GNU is very likely not a good license for a cryptocurrency wallet. question about audits is reasonable.

This comment has been minimized.

TheNikomo replied Jul 20, 2017

@benjyz You would not have access to this code if the no-warranty clause did not exist. Nobody's going to accept personal responsibility for freely-provided code that they may or may not be properly compensated for writing.

I'm also not seeing you volunteer to provide cash to insure every single deployed multisig contract.

This comment has been minimized.

maaaaark replied Jul 20, 2017

@admazzola I understand your frustration, but here's why I think it's being misdirected. This is an open source project that offers a wallet. Ethereum-based blockchain projects are choosing to run their ICO's on a multi-sig wallet that this project generates. Don't you think it should be up to the team behind the ICO to do whatever necessary to make sure that their ICO smart contract is secure?

This comment has been minimized.

dissaranged replied Jul 20, 2017

would it make sense to have solidity default each function, that is not explicit external or public to internal?

Then this would not have happend :)

This comment has been minimized.

persicsb replied Jul 21, 2017

Internal is default per the docs.
"By default, function types are internal, so the internal keyword can be omitted."
http://solidity.readthedocs.io/en/develop/types.html#function-types

Is it a compiler bug?

This comment has been minimized.

SCBuergel replied Jul 21, 2017

AFAIK that is not true and a documentation bug...
I reported this: ethereum/solidity#2617

This comment has been minimized.

admazzola replied Jul 21, 2017

This is only the opinion of one person but I also feel as though it would be prudent to make the functions default to internal in the next version of the solidity compiler. This is the way Java works; classes default to private if it is not explicitly stated to make them public. See you can't get in trouble with a private method, it just wont work at all.

arkpar added a commit that referenced this pull request Jul 19, 2017

arkpar added a commit that referenced this pull request Jul 19, 2017

[beta] Backported wallet fix (#6105)
* Fix initialisation bug. (#6102)

* update wallet library modifiers (#6103)
@3esmit

This comment has been minimized.

3esmit commented on js/src/contracts/code/wallet.js in e06a1e8 Jul 20, 2017

maybe change version to signal this important fix?

@ngotchac ngotchac deleted the multisig-fix branch Aug 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment