1

When reviewing Gnosis Multisig wallet (https://github.com/gnosis/MultiSigWallet/blob/master/contracts/MultiSigWallet.sol#L184) I notice that the comment for submitTransaction function says that Allows an owner. However, on the function declaration, it doesn't have ownerExists requirement. So does this mean that the comment should be allows anyone to?

    /// @dev Allows an owner to submit and confirm a transaction.
    /// @param destination Transaction target address.
    /// @param value Transaction ether value.
    /// @param data Transaction data payload.
    /// @return Returns transaction ID.
    function submitTransaction(address destination, uint value, bytes data)
        public
        returns (uint transactionId)
    {
        transactionId = addTransaction(destination, value, data);
        confirmTransaction(transactionId);
    }

The same goes to the function executeTransaction. The comment says Allows anyone, but it has ownerExists on the function declaration.

    /// @dev Allows anyone to execute a confirmed transaction.
    /// @param transactionId Transaction ID.
    function executeTransaction(uint transactionId)
        public
        ownerExists(msg.sender)
        confirmed(transactionId, msg.sender)
        notExecuted(transactionId)
    {
        if (isConfirmed(transactionId)) {
            Transaction storage txn = transactions[transactionId];
            txn.executed = true;
            //if (external_call(txn.destination, txn.value, txn.data.length, txn.data))
            if (txn.destination.call.value(txn.value)(txn.data))
                Execution(transactionId);
            else {
                ExecutionFailure(transactionId);
                txn.executed = false;
            }
        }
    }

Since gnosis multisig contract seems to be the most secure and famous for holding a lot of funds. I doubt that this is a bug. but I wonder if my understanding is correct about ownerExists requirement.

1

Function submitTransaction doesn't have the ownerExists requirement.

But it calls function confirmTransaction which does.

| improve this answer | |
  • Oh I see, thank you, I missed that. How about executeTransaction? – bbnn Apr 23 at 16:39
  • @bbnn: That one puzzles me too. I've never noticed that before, and I've worked quite a bit with this contract. I guess that they've intended it to be used by anyone, because it can be called only after a sufficient number of authorized signers have confirmed the transaction, so it really doesn't matter who executes it (that user cannot change the transaction at this point, only to pay for its execution). Then, perhaps they've changed their mind on it for some reason, and forgot to update the comment accordingly. Or perhaps this additional requirement is just an unfortunate copy/paste mistake. – goodvibration Apr 23 at 16:44
  • I see.. so it's a comment bug.. – bbnn Apr 23 at 16:46
  • @bbnn: I didn't say that explicitly, but this function can clearly be called only by authorized signers, and not by anyone as the comment states. – goodvibration Apr 23 at 16:48
  • github.com/gnosis/MultiSigWallet/issues/218 somebody else seems to have noticed it too.. – bbnn Apr 23 at 16:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.