This audit was performed by Martin Holst Swende.
Based on the following commit-hash:
https://github.com/ConsenSys/MultiSigWallet/commit/8fd9e0f97572b0e7a2ae4f50233168789162d96d
function confirmTransaction(bytes32 transactionHash) does not require an existing transaction, corresponding to transactionHash. It adds confirmation and performs execution (if confirmed).
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.
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.
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);
}
}
It was found possible for existing owners to create circumstances where they can gain complete control of the multisig wallet.
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.):
B:submitTransactionwith a call to<address>.addOwner(B2).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.
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);
}
- The method
getOwners()copies the data into a separate array. I don't see the point in doing that, compared to just returningowners.
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.
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
}
}
}
}
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.
Enforce that 0 cannot be set as owner
It was found possible to 'replay' transactions across several wallets with shared ownership.
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.
The probably simplest way would be to include the from (current wallet address) as a part of the txHash.
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.
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).
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.
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
valueexceedsbalance
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.
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);
}
calcTransactionHashcould be better placed in the mainMultiSigWallet.sol, since it may be usable there aswell.- The functionality to
submit/confirmpre-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. - It's unclear how users are supposed to use the functionality to pre-sig transactions, since the
eth_signmethod 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.