search

EthereumCommonwealth / ethereum-classic-multisig

NOTE: This wallet is not intended to work with abstract transferrable goods such as tokens, DexNS or ENS names.
BSD 4-Clause "Original" or "Old" License
3 stars 1 forks source link

A single owner can disrupt a vote for a new daily limit #10

Open jelisdeg opened 6 years ago

jelisdeg commented 6 years ago

Given: A MultisigWallet with three Owners: Alice, Bob and Mallory. The wallet is configured to require two confirmations.

Alice and Bob want to change the Daily Limit to 1 ETC, which should be possible, because with 2 confirmations they can approve this change. Mallory does not want to change the daily limit and can prevent it from happening:

  1. Alice calls setDailyLimit(1000000000000000000) Internally Shareable.confirmAndCheck is called and sets up a pendings entry for the Message Hash message_hash_alice := keccak256(msg.data) which says: yetNeeded = 1 ownersDone Bit Matrix: Alice has approved the transaction, Bob and Mallory have not yet approved confirmAndCheck also issues the event Confirmation(Address of Alice, message_hash_alice).

  2. Before Bob calls setDailyLimit(1000000000000000000), Mallory calls confirm(message_hash_alice). confirmAndCheck determines the required confirmations have been provided, deletes the pendings array entry - i.e. finishes the approval process - and returns true. The body of confirm executes but does nothing, because there is no entry in txs[message_hash_alice].

The same issue applies to resetSpentToday()

Possible Solution: Do not share the same pendings array for all functions. Use one array for the function pair execute/confirm. Another one for setDailyLimit And another one for resetSpentToday That way they cannot interfere with each other.

Dexaran commented 6 years ago

Confirmed. The above possibility to interrupt a set of multisig operations does exist.

At the other hand, this opportunity is not a security issue, a vulnerability or an explicit bug because this can not result in a loss of funds or violate access restriction rules. I can also say that from the point of using the wallet as a depository of official donations funds, the reported issue does not pose any threat as well.

I'm not the developer of the multisig wallet and I tend to call it an "undocumented opportunity" or a minor code flaw. Let's report the issue to OpenZeppelin and if they confirm that this is a bug then I will pay the reward of $100 according to point 3 of the bug bounty: "code flaws that can not cause a direct loss of funds."

jelisdeg commented 6 years ago

I don't think it makes sense to open an issue over at the openzeppelin github repository. They abandoned this MultisigWallet implementation in favor of ConsenSys/MultiSigWallet in summer 2017. See commit https://github.com/OpenZeppelin/zeppelin-solidity/commit/58e2e4d742f4f1e227eeab52c31c33dfce8c1efb#diff-ae500f8da61a858eda478e6faf931992

A step you might consider as well. Using a contract that has proven its strength seems safer than relying on a contract, that has been abandoned, has scary looking code that triggers a lot of warnings in Remix and has been "analyzed" by a solidity amateur/newcomer like me and maybe some other random guys.

Regarding the bounty. It made me have a deep look at the contract in the first place. But then it turned out to be an interesting riddle, thinking about what could possibly go wrong. So thanks for some nice puzzle solving hours :) I'd appreciate the bounty, but i'd also be fine, if you use it to pay your bills for another day or two so you can continue to work on EthereumCommonwealth.

Keep it up!