Skip to content

Instantly share code, notes, and snippets.

@holiman

holiman/MultisigWalletAudit.md

Last active February 3, 2021 02:37
Show Gist options
  • Save holiman/77dfe5addab521bf28ea552591ef8ac4 to your computer and use it in GitHub Desktop.
Save holiman/77dfe5addab521bf28ea552591ef8ac4 to your computer and use it in GitHub Desktop.
Download ZIP
This is an audit of the Consensus multisig wallet, by Martin Holst Swende
Raw
MultisigWalletAudit.md

This audit was performed by Martin Holst Swende.

Based on the following commit-hash:

https://github.com/ConsenSys/MultiSigWallet/commit/8fd9e0f97572b0e7a2ae4f50233168789162d96d

1. MultiSigWallet

1.1 Ambiguous confirmations (Low)

function confirmTransaction(bytes32 transactionHash) does not require an existing transaction, corresponding to transactionHash. It adds confirmation and performs execution (if confirmed).

Example

For simplicity, assume it's a 2/1 wallet.

An owner can confirm a non-existing hash of a transaction, and trigger execution. The execution would create a 'blank' transaction ( to 0x0... with 0 balance) and emit a Execution-event.

This behaviour may fool a Dapp which is listening for a particular transaction hash.

Example 2

In another variant, consider a 2/2 wallet. One owner can confirm a non-existing transaction, and ask the other owner to confirm it. The second owner maybe only takes a peek at transactionHashes[hash] and see that it's a 0-value transaction with no data, and confirms.

Aside from triggering a 'blank' transaction, the first owner can now submit the real transaction via submitTransaction. Since destination on the existing 'blank' tx is 0, the transaction will be added in addTransaction, and executed will be reset to false. However, the previous confirmations still exist, and the transaction will be executed immediately.

Recommended fix:

Verify that transaction exist before accepting confirmation.

function confirmTransaction(bytes32 transactionHash)
    public
    ownerExists(msg.sender)
{
 	if(transactions[transactionHash].destination == 0) throw;

    addConfirmation(transactionHash, msg.sender);
    executeTransaction(transactionHash);
 	}
}

1.2. Wallet hijack (Medium)

It was found possible for existing owners to create circumstances where they can gain complete control of the multisig wallet.

Details

In certain cases, it's desirable for users to swap owner-addresses. Typically, this could be the case if one owner suspects a key of being compromised. The wallet is not equipped with any functionality to replace an owner, leading to the following scenario during owner-swap (in the case of a 2/2 wallet, with owners A and B. B wishes to swap to B2.):

  1. B: submitTransaction with a call to <address>.addOwner(B2).
  2. A: confirmTransaction(<hash>).
  • executeTransaction(<hash)
  • addOwner(B2) executed

There's a similar flow required for removing an owner. However, at this point, it's possible for B to take over the wallet, by simply not submitting/confirming the removal of B. At this point, B can instead choose to remove A with his two keys.

And vice versa, if A insists on first removing address B, he can simply choose not to add back B2 after B has confirmed the removal of B.

In conclusion: There is no way for two parties to trustlessly swap a key.

Recommended fix:

Implement replaceOwner:

function replaceOwner(address owner, address newOwner)
    public
    onlyWallet
    ownerDoesNotExist(newOwner)
    ownerExists(owner)
{
    isOwner[owner] = false;
    for (uint i=0; i<owners.length - 1; i++)
        if (owners[i] == owner) {
            owners[i] = newOwner
            break;
        }
    isOwner[newOwner] = true;
    OwnerRemoval(owner);
    OwnerAddition(newOwner);
}

1.3 Other

  • The method getOwners() copies the data into a separate array. I don't see the point in doing that, compared to just returning owners.

2. MultiSigWalletWithDailyLimit

2.1 Partial DoS (Low/Note)

An owner (or previous owner) can block the daily limit withdrawals indefinitely.

The executeTransaction checks if the transaction is under the daily limit. The underLimit updates the spentToday counter.

Let's say there's a transaction submitted (but not necessarily confirmed by the other owners) of 1 ether, to a griefing recipient which always throws. Let's say the daily limit is 1 ether.

An attacker can now call executeTransaction(hash) every day. This will set spentToday to 1 ether, and even though there's an ExecutionFailure and tx.executed is set back to false, the spentToday is still at 1 ether, preventing other owners from using the daily limit.

Recommended fix:

Set back spentToday to previous value in case of failed transaction.

Example code (not necessarily correct):

function executeTransaction(bytes32 transactionHash)
    public
    notExecuted(transactionHash)
{
    Transaction tx = transactions[transactionHash];
    if (isConfirmed(transactionHash) || tx.data.length == 0 && underLimit(tx.value)) {
        tx.executed = true;
        if (tx.destination.call.value(tx.value)(tx.data))
            Execution(transactionHash);
        else {
            ExecutionFailure(transactionHash);
            tx.executed = false;
        }
    }else if ( tx.data.length == 0 && tx.value < dailyLimit)
    {	
    	//Check daily limit
        if (now > lastDay + 24 hours) {
            lastDay = now;
            spentToday = 0;
        }
        if (tx.value < dailyLimit){
	        spentToday += tx.value;
            if (tx.destination.call.value(tx.value)(tx.data))
                Execution(transactionHash);
            else {
                ExecutionFailure(transactionHash);
                tx.executed = false;
                spentToday -= tx.value
            }
        }
    }
}

3. MultiSigWalletWithPreSign.sol

3.1 Ecrecover failure (note)

modifier signaturesFromOwners(bytes32 transactionHash, uint8[] v, bytes32[] rs) {
    for (uint i=0; i<v.length; i++)
        if (!isOwner[ecrecover(transactionHash, v[i], rs[i], rs[v.length + i])])
            throw;
    _;

ecrecover returns 00... on failure to recover sig. So if 00.. is an owner, anyone can confirm. Might be nothing, could potentially be something in combination with other flaws.

Recommended fix

Enforce that 0 cannot be set as owner

3.2 Cross-wallet replay (Medium)

It was found possible to 'replay' transactions across several wallets with shared ownership.

Details

function submitTransactionPreSigned(address destination, uint value, bytes data, uint nonce, uint8[] v, bytes32[] rs)
    public
    returns (bytes32 transactionHash)
{
    transactionHash = addTransaction(destination, value, data, nonce);
    confirmTransactionPreSigned(transactionHash, v, rs);
}

Let's take the example of three owners, A,B and C, on wallet X . Additionally, you have A , B and D which have the multisig Y. Both are 2/3 multisig.

Let's assume that A and B confirm a tx on X. An attacker can now replay that one on multisig Y.

Suggested fix

The probably simplest way would be to include the from (current wallet address) as a part of the txHash.

3.3 Transaction malleability (Medium/High)

The method submitTransactionPreSigned(address destination, uint value, bytes data, uint nonce, uint8[] v, bytes32[] rs) and confirmTransactionPresigned are is susceptible to spurious attacks due to malleability.

Example 1 (confirmTransactionPreSigned)

Consider the case of a 1/1 multisig, owned by A. In order to submit a transaction, anyone can invoke submitTransactionPresigned or confirmTransactionPreSigned with valid (signed) data.

A normal Ethereum transaction is on the following form:

RLP{
    nonce    // big_endian_int
    gasprice // big_endian_int
    startgas // big_endian_int
    to       // utils.address
    value    // big_endian_int
    data     // binary
    v
    r
    s
}

An attacker can simply take an RLP-blob from a previous ethereum transaction submitted by A, and simply rearrange the data, so that RLP(nonce,gasprice,startgas,to,value,data) becomes destination, value,data,nonce

Here is an example of a valid ETH transaction:

   "transaction" : {
        "data" : "",
        "gasLimit" : "0x5208",
        "gasPrice" : "0x01",
        "nonce" : "0x00",
        "r" : "0x98ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4a",
        "s" : "0x2887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a3",
        "to" : "0x000000000000000000000000000b9331677e6ebf",
        "v" : "0x1c",
        "value" : "0x0a"
    }
    "hash" : "99354ff47d8a69e44f42fc706213a8b3ba1285184ce53611572a089db2b2d9f1",
    "sender" : "aeed6ced7a2af49a8ff0dca6a0811e40462a6d4d",

RLP-encoded:

0xf85f800182520894000000000000000000000000000b9331677e6ebf0a801ca098ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4aa02887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a3

The unsigned RLP-data of that transaction is:

dc800182520894000000000000000000000000000b9331677e6ebf0a80

And the raw hash, to be signed, is

9f8e5c24b9b3a0664a9f8b358c07ea710e5a40b82d40abf75f68029708744dda

That means that the following can be submitted by anyone:

confirmTransactionPreSigned(
	"0x9f8e5c24b9b3a0664a9f8b358c07ea710e5a40b82d40abf75f68029708744dda",
	[28],
	["0x98ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4a",
	"0x2887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a3"]
)

This will trigger the following Events:

Confirmation[
  "0xaeed6ced7a2af49a8ff0dca6a0811e40462a6d4d",
  "0x9f8e5c24b9b3a0664a9f8b358c07ea710e5a40b82d40abf75f68029708744dda"
]
Execution[
  "0x9f8e5c24b9b3a0664a9f8b358c07ea710e5a40b82d40abf75f68029708744dda"
]

And a 'blank' transaction to 0x0 will be generated (see separate issue).

Example 2 (submitTransactionPreSigned)

In order to pull off a similar attack against submitTransactionPreSigned, it's a bit more complicated. As an example, I picked a random transaction which had some data in it 0x2e0e8da7bbac0e270a77425bb1c1b1029e3e3c37aa8382ba8c9046da9d0d5d0f

The sender was 0xcf40d0d2b44f2b66e07cace1372ca42b73cf21a3, so first of call, create a 1/1 wallet with that address as owner.

The unsigned rlpdata is of the transaction is:

f90127821ce185098bca5a00830249f08080b90115606060405260008054600160a060020a0319163317905560f2806100236000396000f3606060405260e060020a6000350463f5537ede8114601c575b6002565b3460025760f06004356024356044356000805433600160a060020a039081169116141560ea5783905080600160a060020a031663a9059cbb84846000604051602001526040518360e060020a0281526004018083600160a060020a0316815260200182815260200192505050602060405180830381600087803b1560025760325a03f1156002575050604080518481529051600160a060020a0386811693508716917fd0ed88a3f042c6bbb1e3ea406079b5f2b4b198afccaa535d837f4c63abbc4de6919081900360200190a35b50505050565b00

When we break that data up, we wind up with the following values:

  • address: f90127821ce185098bca5a00830249f08080b901
  • value: 15606060405260008054600160a060020a0319163317905560f2806100236000
  • data: 396000f3606060405260e060020a6000350463f5537ede8114601c575b6002565b3460025760f06004356024356044356000805433600160a060020a039081169116141560ea5783905080600160a060020a031663a9059cbb84846000604051602001526040518360e060020a0281526004018083600160a060020a0316815260200182815260200192505050602060405180830381600087803b1560025760325a03f1156002575050604080518481529051600160a060020a0386811693508716917fd0ed88a3f042c6bbb1e3ea406079b5f2b4b1
  • nonce: 98afccaa535d837f4c63abbc4de6919081900360200190a35b50505050565b00

And the signature values:

  • v, 1b
  • r: 349f59b9b57227fd84178f0d3ca847327765a55db7bf0075b634178f57863722
  • s: 7c643e514fe2ed0d29c34063ebb46e34e1c0a7784c4e1aeecb6ff8a763fd0fa5

Note; the online solidity compiler does not encode "0xFF" as binary data for <bytes>, so it needs to be expressed as arrays for that to work.

Verify calcTransactionHash with the following values:

"0xf90127821ce185098bca5a00830249f08080b901","0x15606060405260008054600160a060020a0319163317905560f2806100236000",[57, 96, 0, 243, 96, 96, 96, 64, 82, 96, 224, 96, 2, 10, 96, 0, 53, 4, 99, 245, 83, 126, 222, 129, 20, 96, 28, 87, 91, 96, 2, 86, 91, 52, 96, 2, 87, 96, 240, 96, 4, 53, 96, 36, 53, 96, 68, 53, 96, 0, 128, 84, 51, 96, 1, 96, 160, 96, 2, 10, 3, 144, 129, 22, 145, 22, 20, 21, 96, 234, 87, 131, 144, 80, 128, 96, 1, 96, 160, 96, 2, 10, 3, 22, 99, 169, 5, 156, 187, 132, 132, 96, 0, 96, 64, 81, 96, 32, 1, 82, 96, 64, 81, 131, 96, 224, 96, 2, 10, 2, 129, 82, 96, 4, 1, 128, 131, 96, 1, 96, 160, 96, 2, 10, 3, 22, 129, 82, 96, 32, 1, 130, 129, 82, 96, 32, 1, 146, 80, 80, 80, 96, 32, 96, 64, 81, 128, 131, 3, 129, 96, 0, 135, 128, 59, 21, 96, 2, 87, 96, 50, 90, 3, 241, 21, 96, 2, 87, 80, 80, 96, 64, 128, 81, 132, 129, 82, 144, 81, 96, 1, 96, 160, 96, 2, 10, 3, 134, 129, 22, 147, 80, 135, 22, 145, 127, 208, 237, 136, 163, 240, 66, 198, 187, 177, 227, 234, 64, 96, 121, 181, 242, 180, 177],"0x98afccaa535d837f4c63abbc4de6919081900360200190a35b50505050565b00"

Yields : e37e67fc4ae350b9f5f87172518cbb981a0eb54dcc37507e25320fd47e06fe2e . Which is correct.

The submitTransactionPreSigned can be called with the following values:

"0xf90127821ce185098bca5a00830249f08080b901","0x15606060405260008054600160a060020a0319163317905560f2806100236000",[57, 96, 0, 243, 96, 96, 96, 64, 82, 96, 224, 96, 2, 10, 96, 0, 53, 4, 99, 245, 83, 126, 222, 129, 20, 96, 28, 87, 91, 96, 2, 86, 91, 52, 96, 2, 87, 96, 240, 96, 4, 53, 96, 36, 53, 96, 68, 53, 96, 0, 128, 84, 51, 96, 1, 96, 160, 96, 2, 10, 3, 144, 129, 22, 145, 22, 20, 21, 96, 234, 87, 131, 144, 80, 128, 96, 1, 96, 160, 96, 2, 10, 3, 22, 99, 169, 5, 156, 187, 132, 132, 96, 0, 96, 64, 81, 96, 32, 1, 82, 96, 64, 81, 131, 96, 224, 96, 2, 10, 2, 129, 82, 96, 4, 1, 128, 131, 96, 1, 96, 160, 96, 2, 10, 3, 22, 129, 82, 96, 32, 1, 130, 129, 82, 96, 32, 1, 146, 80, 80, 80, 96, 32, 96, 64, 81, 128, 131, 3, 129, 96, 0, 135, 128, 59, 21, 96, 2, 87, 96, 50, 90, 3, 241, 21, 96, 2, 87, 80, 80, 96, 64, 128, 81, 132, 129, 82, 144, 81, 96, 1, 96, 160, 96, 2, 10, 3, 134, 129, 22, 147, 80, 135, 22, 145, 127, 208, 237, 136, 163, 240, 66, 198, 187, 177, 227, 234, 64, 96, 121, 181, 242, 180, 177],"0x98afccaa535d837f4c63abbc4de6919081900360200190a35b50505050565b00",[27],["0x349f59b9b57227fd84178f0d3ca847327765a55db7bf0075b634178f57863722","0x7c643e514fe2ed0d29c34063ebb46e34e1c0a7784c4e1aeecb6ff8a763fd0fa5"]

And the attack almost works.

What prevents the attack above, is that he nonce used in our ethereum-derived transaction becomes 98afccaa535d837f4c63abbc4de6919081900360200190a35b50505050565b00, but should be 0, which is checked by the modifier validNonce, used to protect addTransaction:

modifier validNonce(address destination, uint value, bytes data, uint nonce) {
    if (nonce > nonces[keccak256(destination, value, data)])
        throw;
    _;
}

This means that in order to pull this off, we need to find a transaction which meets the contraint that the unsigned rlpdata ends with 32 0-bytes. Since the rlpdata ends with the actual data, it means that it may be possible to 'lure' a victim to engage with a particular ABI and invoke a seemingly innocous method which sends 0x00..00 as the (last part of) transaction payload.

A second requirement is that value must wind up within the multisig wallet balance.

Recommended fix

The pre-signed confirmations and pre-signed submission functionality is far too lenient on syntax. An ethereum-transaction has the syntactic rule that it must be a valid RLP-encoded transaction.

The Multisig wallet transcation, on the other hand, has no syntactic validation: any blob of data of sufficient length, is a valid transaction:

  • destination : 20 bytes, no format
  • value : 32 bytes, no format
  • data : N bytes, no format
  • nonce : 32 bytes, no format

Any signed piece of data, exceeding 84 bytes, is syntactically valid!

The only reason for an attack to fail are semantic rules;

  • If nonce is not correct
  • If value exceeds balance

I recommend either not having this functionality in place at all, or implementing syntactical constraints to disambiguate between Multisigwallet-transactions and other types of signed data.

3.4 Pre-signed Confirmation functional notes (Note)

function confirmTransactionPreSigned(bytes32 transactionHash, uint8[] v, bytes32[] rs)
    public
    signaturesFromOwners(transactionHash, v, rs)
{
    for (uint i=0; i<v.length; i++)
        addConfirmation(transactionHash, ecrecover(transactionHash, v[i], rs[i], rs[i + v.length]));
    executeTransaction(transactionHash);
}

First of all, if any owner has already confirmed, this will throw, and have to be redone. So if you have a list of 20 persons to confirm, and collect their sigs, if one of them already confirmed you need to weed that one out and do it all again. Minor note.

Secondly, it's wasting a lot of gas, calling ecrecover on each signature both during signaturesFromOwners and during confirmation loop. It would probably make sense to inline the modifier instead, and use only one loop:

function confirmTransactionPreSigned(bytes32 transactionHash, uint8[] v, bytes32[] rs)
    public
    signaturesFromOwners(transactionHash, v, rs)
{

    for (uint i=0; i<v.length; i++){
    	owner = ecrecover(transactionHash, v[i], rs[i], rs[v.length + i])
        if (!isOwner[owner])
            throw;
        addConfirmation(transactionHash, ecrecover(transactionHash, v[i], rs[i], rs[i + v.length]));
    }            
    executeTransaction(transactionHash);
}

3.5 Other (Low/Note)

  1. calcTransactionHash could be better placed in the main MultiSigWallet.sol, since it may be usable there aswell.
  2. The functionality to submit/confirm pre-signed transactions makes the revokation of confirmations obsolete: any owner who has given out a signed confirmation can still revoke the confirmation, but anyone can re-submit his signed confirmation afterwards. This should be clearly documented.
  3. It's unclear how users are supposed to use the functionality to pre-sig transactions, since the eth_sign method now prepends a static string before signing. So in order to be able to use existing RPC-methods that needs to be taken into account.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment