Solidity: Remove tx.origin

[PeterBorah]

Summary

tx.origin is a security vulnerability, breaks compatibility with other contracts including security contracts, and is almost never useful. Removing it would make Solidity more user-friendly. If there are exceptional cases where access to the transaction origin is needed, a library using in-line assembly can provide it.

The Problems

1) tx.origin is a security vulnerability. As we recently saw with the Mist wallet, using tx.origin makes you vulnerable to attacks comparable to phishing or cross-site scripting. Once a user has interacted with a malicious contract, that contract can then impersonate the user to any contract relying on tx.origin.

2) tx.origin breaks compatibility. Using tx.origin means that your contract cannot be used by another contract, because a contract can never be the tx.origin. This breaks the general composability of Ethereum contracts, and makes them less useful. In addition, this is another security vulnerability, because it makes security-based contracts like multisig wallets incompatible with your contract.

3) tx.origin is almost never useful. This is the most subjective point, but I have yet to come across a use of tx.origin that seemed legitimate to me. I welcome counter-examples, but I've written dozens or hundreds of smart contracts without needing it, and I have never heard of anyone else needing it either.

Rationale for Removal

Solidity's design philosophy is to prioritize security and reliability over expressiveness. In other cases where behavior is unreliable, Solidity does not expose it. (For instance, there is no way to call an external contract and retrieve the return value if the signature is not known ahead of time.)

The "escape clause" is in-line assembly, which allows the creation of libraries to do anything expressible as EVM assembly. Behavior that is unsafe and unreliable is best kept in libraries, rather than given to all users as part of the core language.

[aakilfernandes]

SafeMarket currently has a vulnerability due to use of tx.origin. Thankfully, Peter explained this bug to me otherwise I would have fallen into the trap.

[chriseth]

I'm not sure removing is a good idea, but we can add a warning.

[PeterBorah]

Why not?

[chriseth]

Because it will break backwards-compatibility. If you are saying that a warning is not enough and will not be noticed, we should revisit how people compile their contracts.

[chriseth]

If someone wants to implement that, please take a look at https://github.com/ethereum/solidity/pull/677 which is very similar. This one has to go into visit(MemberAccess ...).

[PeterBorah]

Fair enough. Is Solidity committed to backwards-compatibility in general?

I would recommend at least adding it to the list of things to change when you do the next breaking change. Any code that would be broken by this needs to be broken, because it is almost certainly insecure in some way.

[redsquirrel]

Any code that would be broken by this needs to be broken, because it is almost certainly insecure in some way.

Good point here 👆

[VoR0220]

I think @PeterBorah makes a good point here. I think backwards compatibility for this security flaw might be worth breaking just this time.

[pipermerriam]

So far the only times I've used tx.origin are:

1. Bypassing stack depth checking
2. Source of entropy.

[slothbag]

I have contracts that ripple calls up a chain of contracts.. and I need to know which account was the originator. I use tx.origin for that.

[PeterBorah]

@slothbag Regardless of the outcome of this pull request, double and triple check that you're actually secure there. Chances are, there are ways for malicious contracts to do unexpected things by impersonating your users or your contracts. Is it ok if an attacker is able to call any of your functions at any time, with any arguments, while appearing to be a legitimate user? If not, tx.origin isn't safe.

Also consider whether it's ok that your users can't use multisig wallets, and can't be DAOs or anything like that.

If you're interested, I'd be happy to take a look at your contracts and offer advice for structuring your contracts so you don't need tx.origin. I am on a quest to eliminate it from existence!

[slothbag]

Thanks @PeterBorah I will definitely revisit my usage of it.

[Nashatyrev]

One of our contracts allows registering only non-contract address. We are checking this with msg.sender == tx.origin

[PeterBorah]

@Nashatyrev: Why do you want to do this? It vastly reduces your user's security options, because they can't use multisig, key revocation, or other contract-based security systems. As a user, I would not appreciate being forced to use a dangerous method (a bare private/public keypair) for interacting with a contract.

Additionally, this won't work the way you expect it to after EIP 101.

[Nashatyrev]

E.g. you have a Token contract (address -> balance), and you want to get fees from transferring tokens between owners. If you allow a contract to own a token account then those token can be locked and transferred between owners without any fees (i.e. kind of derivative token contract). Non-contract only accounts prevent this. As a user you may grant any contract with access to your account (which still prevents token locking) for the benefits you've mentioned.

Not sure why EIP 101 should break this? The transaction will still have a signer and the contract invocation will still have a caller.

[PeterBorah]

As a user you may grant any contract with access to your account (which still prevents token locking) for the benefits you've mentioned.

This isn't true. There will still be a private key somewhere with access to the tokens, which means there's no way to use any other form of security exclusively. If someone gets access to the private key, it's game over.

(It's also just annoying that I have to generate a private key just for your one dapp, rather than using the security system I use for literally everything else.)

Not sure why EIP 101 should break this? The transaction will still have a signer and the contract invocation will still have a caller.

I don't know exactly what the semantics will be, but either tx.origin will be the origin address, in which case it is useless for telling who sent it, or it will be the address of the contract that pays the gas, in which case it could easily be the sort of contract you're hoping to prevent.

[Nashatyrev]

You may generate a key, create an account, grant account access to any contract you like and immediately destroy the key.
The trick is you can't prove others you have destroyed the key and thus the tokens can't be assumed locked. So any token transfer should be still performed via the Token contract and transfer fees get paid.
Can you please tell what security system are you referring?

EIP 101: Do you mean this change is going to be VM backward incompatible?

[PeterBorah]

You may generate a key, create an account, grant account access to any contract you like and immediately destroy the key.

Yeah, that works I guess, though you have to hope that you didn't leak the key during the creation and signing phase. Not as secure as I'd like, but not terrible.

Can you please tell what security system are you referring?

I don't have one set up yet, so I was referring to the hypothetical future. But any security system based on smart contracts, which will be pretty much any of them. For instance, anything with a multisig component, or anything where you can change keys or where you have "emergency override" keys. We're building this sort of system at Ownage for our users, and there will be lots more coming out in the near-to-medium-term future. The Mist multisig wallets are a simple example of this pattern.

EIP 101: Do you mean this change is going to be VM backward incompatible?

Well, it's definitely backwards incompatible (until now all transactions had to be signed in a specific format, after EIP 101 any transaction is potentially valid regardless of signature). But in this specific case, it's not the backwards incompatibility that's the problem, but the general functionality implied by the EIP. Since you can have your "account" work based on any code you write, the "account" can become the sort of derivative you don't want.

[axic]

@PeterBorah:

I would recommend at least adding it to the list of things to change when you do the next breaking change.

It would definitely make sense starting a list of possible breaking changes and have them in the next version. Since we're following semver, 0.4.x could have breaking changes.

I would include not only security related changes, but remove any possible dead weight which could be a burden in the future evolution of the language.

@chriseth would it make sense having a separate issue opened for it or a git issue tag is sufficient? Judging by a quick browse there are plenty of proposed breaking changes.

[chriseth]

Yes, such a list makes sense. I actually prefix all PRs that introduce breaking change with BREAKING.

As far as tx.origin is concerned, I am still not convinced that banning it to inline assembly is a good thing. If you do access control, then every single example uses msg.sender and not tx.origin. We also don't forbid users to do access control using passwords in contracts, but we tell users in the documentation not to do it.

I'm fine with adding a static analysis check in browser-solidity to search for tx.origin, but I would not want to remove it from the language.

[PeterBorah]

We also don't forbid users to do access control using passwords in contracts, but we tell users in the documentation not to do it.

There are lots of things that Solidity doesn't allow you to do. It's a statically typed language! This is the same sort of thing: tx.origin is a bug that is can be detected at compile time.

I'm sympathetic to the argument that power is more important than safety, but that doesn't seem to be Solidity's philosophy in any other situation.

[chriseth]

Replaced by https://github.com/ethereum/browser-solidity/issues/173

[akrolak]

Actually I have opposite problem, I want to use tx.origin for personal account but it returns contract address in spoke and hub model :(

[oojr]

@PeterBorah simple use case for tx.origin I want a contract to approve and transferFrom tokens on my behalf, is calling two different contracts more user friendly?

[PeterBorah]

I don't know if it's more user-friendly, but it's definitely more secure. If your token contract checks tx.origin instead of msg.sender, then any contract you ever call can steal your tokens, even if you thought you were just breeding some digital cats.

User friendliness is easy to handle in the interface. No reason to compromise security to do it.

[oojr]

any contract can change ownership and self destruct if modifiers are not in place, how will someone steal my tokens if I am checking if msg.sender is an approved contract?

remoteApprove(_spender){
  require(approvedReceiver(msg.sender));
  allowance[tx.origin][_spender] = _value;
}

@PeterBorah I agree though security shouldn't be comprimised, and it could probably be easier done in the interface, its better to keep the Solidity attack vectors small

[PeterBorah]

That definitely helps, but then the attack flow is:

You -> EvilCryptoKitties -> ApprovedReceiverContract -> Token

You're the tx.origin, so EvilCryptoKitties can act on your behalf and send your tokens to themselves.

This doesn't work if ApprovedReceiverContract checks msg.sender, but at that point you're just trusting all of the security to ApprovedReceiverContract, so why not just remove the false security provided by tx.origin like this:

```
remoteApprove(_spender, _owner){
require(approvedReceiver(msg.sender));
allowance[_owner][_spender] = _value;
}

[oojr]

why would I send a transaction to EvilCryptoKitties in that contract pretend they have

function payable(){
  owner.transfer(msg.value * 100)
}

In this scenario tx.origin is not the problem but interacting with the bad contract is

the flow is Me -> ApprovedReceiveContract -> Token

it is still two transactions either way, so its better to have the safer msg.sender

[PeterBorah]

You would call EvilCryptoKitties because you want to breed lovely digital kitties, of course.

They can't do your owner.transfer(msg.value * 100) in any other case because the contract doesn't have the right to transfer on behalf of your account. The only way they get that ability is if you use the insecure tx.origin.

[oojr]

Yes my example actually wouldnt work without EvilCrpytoKitties is a bad contract regardless any behavior is malicious I dont think tx.origin will protect that but can prevent middleman contract attacks in an automated flow

[PeterBorah]

No, it mostly can't do bad things if contracts follow basic security practices like not using tx.origin. It can't act on your behalf, or move your ether, unless you choose to send it ether. It can only change its own internal state and make calls that set itself as the msg.sender. You don't normally have to audit a smart contract to make sure it can't steal your tokens, you just trust that the tokens have reasonable security and your tokens will be safe regardless.

What did you think of my suggestion that you just trust ApprovedReceiveContract entirely, rather than having the useless tx.origin check?

[oojr]

Maybe an audit is not needed but verification will help to be on the safe side. For example ICO website can get hacked and a contract address can be changed to a malicious contract that accepts Ether and doesnt transfer tokens,

In your example the owner param can be changed by the malicious contract, in my suggestion, tx.origin can not be changed

interface token (){
  remoteApprove(address spender, uint value)
}

contract TokenReceiver {
 function TokenReceiver(address addressOfToken){
   tokenReward = token(addresOfToken)
  }

   function approveRemoteTransfer(uint value){
     tokenReward.remoteApprove(this, value)
  }
}

1) interact approveRemoteTransfer

remoteApprove(_spender){
  require(approvedReceiver(msg.sender));
  allowance[tx.origin][_spender] = _value;
}

1. msg.sender is the contract TokenReceiver, tx.origin is the msg.sender from TokenReceiver

@PeterBorah I will just remain as compliant to ERC-20 token in case tx.origin becomes deprecated, I can also run a rinkeby contract to see if you can exploit it

[HarryR]

There are legitimate use cases for tx.origin, it is silly to try and remove this just because people can shoot themselves in the foot with it. It may be 'bad security practice' with many use cases, but msg.sender isn't appropriate for others and must remain usable when necessary.

For example, I want to verify the call chain of a transaction, to ensure that it originated from a user, and that it passed through exactly two other contracts (which are on my whitelist) before reaching me.

[ytrezq]

@akrolak you’re getting tx.orgin to return a contract address ? I’m really interested. Please explain how you got that problem ! (including through e mail).

[James-Sangalli]

@PeterBorah wouldn't one need tx.origin if they are delegating on behalf of the caller? at the moment it is not possible to keep the msg.sender for the original caller when using a proxy contract for example.

[jooray]

@PeterBorah wouldn't one need tx.origin if they are delegating on behalf of the caller? at the moment it is not possible to keep the msg.sender for the original caller when using a proxy contract for example.

No, you would not need that, even worse, it is really a bad idea.

You should keep msg.sender by using a trusted proxy contract and then you can verify that msg.sender is approved proxy contract and then you have original sender as an argument to transaction.

On the other hand, if you use tx.origin, you allow any contract deployed on the blockchain to be a proxy, meaning someone wants to buy a CryptoDog or play with some stupid game and that contract could act on behalf of the user, if you rely on tx.origin. Please, don't do that.

+1 for removing tx.origin

[HarryR]

On the other hand, if you use tx.origin, you allow any contract deployed on the blockchain to be a proxy, meaning someone wants to buy a CryptoDog or play with some stupid game and that contract could act on behalf of the user, if you rely on tx.origin. Please, don't do that.

This is a valid use case, as I said before - I don't think it's justified to remove useful functionality solely to prevent people from shooting themselves in the foot.

In reposte, I propose to remove malloc from C, because it often leads to memory leaks.