Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multisig support for data in transactions (as long as data.length is not empty) #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add multisig support for data in transactions (as long as data.length is not empty) #62

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions wallet/wallet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,14 +330,16 @@ contract Wallet is multisig, multiowned, daylimit {
if (msg.value > 0)
Deposit(msg.sender, msg.value);
}
// Outside-visible transact entry point. Executes transacion immediately if below daily spend limit.

// Outside-visible transact entry point. Executes transaction immediately if below daily spend limit.
// If not, goes into multisig process. We provide a hash on return to allow the sender to provide
// shortcuts for the other confirmations (allowing them to avoid replicating the _to, _value
// 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),
Copy link
Contributor

@chriseth chriseth Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea this was the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length check can go, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// since we are unable to determine the value outcome of it.
if (underLimit(_value) && !hasCode(_to)) {
SingleTransact(msg.sender, _value, _to, _data);
// yes - just execute the call.
_to.call.value(_value)(_data);
Expand Down Expand Up @@ -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) internal constant returns (bool) {
uint size;
assembly { size := extcodesize(_addr) }
return size > 0;
}

// FIELDS

// pending transactions we have at present.
Expand Down