Add multisig support for data in transactions (as long as data.length is not empty) #62
Conversation
bencxr
changed the title
| @@ -337,7 +337,9 @@ contract Wallet is multisig, multiowned, daylimit { | |||
| // and _data arguments). They still get the option of using them if they want, anyways. | |||
| function execute(address _to, uint _value, bytes _data) external onlyowner returns (bytes32 _r) { | |||
| // first, take the opportunity to check that we're under the daily limit. | |||
| if (underLimit(_value)) { | |||
| // we also must check that there is no data (this is not a contract invocation), | |||
There was a problem hiding this comment.
Every transaction executes code, even if the data is empty.
You can only be sure that no code will execute by checking that extcodesize == 0:
function hasCode(address _addr) returns (bool) {
uint size;
assembly { size := extcodesize(_addr) }
return size > 0;
}
Untested code, though.
There was a problem hiding this comment.
I had no idea this was the case
There was a problem hiding this comment.
thank you, i updated the diff to also check if the other address has code.
I wonder if we still need to check for _data.length here?
There was a problem hiding this comment.
The length check can go, I think.
There was a problem hiding this comment.
Every transaction executes code, even if the data is empty. You can only be sure that no code will execute by checking that extcodesize == 0:
Personally I think this is too restrictive; it would forbid sending to contract wallets. And imo we really should be pressing harder on discouraging contracts from having non-trivial fallbacks; there are other reasons to do that anyway.
There was a problem hiding this comment.
For now I have changed this as @chriseth recommended without the length check. This should be safe and require more signers, which may be since the purpose of the wallet is mostly for multi-sig.
We could also try a .send instead of a call, and if that fails then we could require the multi signer. I'm not sure how safe that is though.
| @@ -373,6 +375,13 @@ contract Wallet is multisig, multiowned, daylimit { | |||
| super.clearPending(); | |||
| } | |||
|
|
|||
| // Used to determine if an address may execute code | |||
| function hasCode(address _addr) returns (bool) { | |||
There was a problem hiding this comment.
Add constant and internal specifiers?..
EDIT: verbose.
There was a problem hiding this comment.
thanks for input - I've updated the lines to be internal constant
|
Pinging @chriseth, any thoughts on this? |
|
I would prefer a full rewrite of the wallet contract. I have seen a pretty simple rewrite, but I'm not sure if it is published yet. |
Currently data transactions with 0 value are always sent out using only a single owner. This is dangerous as it can apply to other tokens (such as DAO) where there is token transfer, but no ETH is transferred.
This change will trigger multisig (multiple owners) required for non-eth transfers (or any transaction including data).
We should do this because the outcome is not possible to identify in this contract, and so it is safer to require multiple signers.
I'm working in another repo where I've added tests for the wallet. You can view the check in and test here:
BitGo/eth-multisig-v2@013bc61