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.

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

Remove tx.origin #683

Closed
PeterBorah opened this Issue Jun 24, 2016 · 34 comments

Comments

Projects
None yet
@PeterBorah

PeterBorah commented Jun 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@aakilfernandes

aakilfernandes Jun 24, 2016

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.

aakilfernandes commented Jun 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jun 24, 2016

Contributor

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

Contributor

chriseth commented Jun 24, 2016

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

@PeterBorah

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah commented Jun 24, 2016

Why not?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jun 24, 2016

Contributor

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.

Contributor

chriseth commented Jun 24, 2016

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 chriseth added the planned label Jun 24, 2016

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jun 24, 2016

Contributor

If someone wants to implement that, please take a look at #677 which is very similar. This one has to go into visit(MemberAccess ...).

Contributor

chriseth commented Jun 24, 2016

If someone wants to implement that, please take a look at #677 which is very similar. This one has to go into visit(MemberAccess ...).

@PeterBorah

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jun 24, 2016

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.

PeterBorah commented Jun 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@redsquirrel

redsquirrel Jun 24, 2016

Contributor

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

Good point here 👆

Contributor

redsquirrel commented Jun 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@VoR0220

VoR0220 Jun 24, 2016

Collaborator

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

Collaborator

VoR0220 commented Jun 24, 2016

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

@pipermerriam

This comment has been minimized.

Show comment
Hide comment
@pipermerriam

pipermerriam Jun 25, 2016

Member

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

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

pipermerriam commented Jun 25, 2016

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

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

This comment has been minimized.

Show comment
Hide comment
@slothbag

slothbag Jun 25, 2016

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.

slothbag commented Jun 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jun 25, 2016

@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!

PeterBorah commented Jun 25, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@slothbag

slothbag Jun 25, 2016

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

slothbag commented Jun 25, 2016

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

@Nashatyrev

This comment has been minimized.

Show comment
Hide comment
@Nashatyrev

Nashatyrev Jul 20, 2016

Member

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

Member

Nashatyrev commented Jul 20, 2016

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

@PeterBorah

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jul 20, 2016

@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.

PeterBorah commented Jul 20, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Nashatyrev

Nashatyrev Jul 20, 2016

Member

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.

Member

Nashatyrev commented Jul 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jul 21, 2016

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.

PeterBorah commented Jul 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Nashatyrev

Nashatyrev Jul 21, 2016

Member

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?

Member

Nashatyrev commented Jul 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jul 21, 2016

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.

PeterBorah commented Jul 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jul 25, 2016

Member

@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.

Member

axic commented Jul 25, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jul 27, 2016

Contributor

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.

Contributor

chriseth commented Jul 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Jul 27, 2016

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.

PeterBorah commented Jul 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
Contributor

chriseth commented Aug 11, 2016

@akrolak

This comment has been minimized.

Show comment
Hide comment
@akrolak

akrolak May 20, 2017

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

akrolak commented May 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oojr

oojr Dec 7, 2017

@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?

oojr commented Dec 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Dec 7, 2017

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.

PeterBorah commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oojr

oojr Dec 7, 2017

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

oojr commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Dec 7, 2017

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;
}

PeterBorah commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oojr

oojr Dec 7, 2017

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

oojr commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Dec 7, 2017

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.

PeterBorah commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oojr

oojr Dec 7, 2017

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

oojr commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@PeterBorah

PeterBorah Dec 7, 2017

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?

PeterBorah commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@oojr

oojr Dec 7, 2017

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

oojr commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@HarryR

HarryR Dec 19, 2017

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.

HarryR commented Dec 19, 2017

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.

@Recmo Recmo referenced this issue May 15, 2018

Merged

Add Validator signature type #561

0 of 8 tasks complete
@ytrezq

This comment has been minimized.

Show comment
Hide comment
@ytrezq

ytrezq Jun 17, 2018

@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).

ytrezq commented Jun 17, 2018

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment