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.

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

ERC: transferFrom Token Standard #3055

Closed
gkucmierz opened this issue Oct 18, 2020 · 16 comments
Closed

ERC: transferFrom Token Standard #3055

gkucmierz opened this issue Oct 18, 2020 · 16 comments
Labels

Comments

@gkucmierz
Copy link

gkucmierz commented Oct 18, 2020

Preamble

Title: transferFrom Token Standard
Author: Grzegorz Kucmierz <gkucmierz@gmail.com>
Type: Informational
Category: ERC
Status: Draft
Created: 2020-10-18
Requires: ERC20

Simple Summary

Allow tokens to be transferred from address, that have no ETH in it to cover gas for transaction, but it provides off-chain signature that is allowing transfer for specified amount and specified recipient.

Abstract

This adds a new function to ERC20 token contracts, transferFrom* which can be called to transfer tokens from another address.

  • This transferFrom have different signature than original ERC20 transferFrom
function transferFrom(address, uint256, uint8, bytes32, bytes32) returns (bool);

Motivation

Using this function it is possible to deposit tokens to contract, and also contract can react to this transfer within one transaction.
Also single address is able to make transfer from any other address by providing additional signature, that confirms validity of this transfer.

Specification

Token

transferFrom

mapping (address => mapping (address => uint256)) public nonceOf;
  
function transferFrom(address recipient, uint256 amount, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    bytes32 hash = keccak256(abi.encodePacked('transferFrom', recipient, amount, nonce));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
}

transferFrom transfers tokens to from, which is calculated using ecrecover
Warning about this function is that created signature will be valid forever, so there should be another function also that limits signature validity to only specified block

transferFromUntil

function transferFromUntil(address recipient, uint256 amount, uint256 untilBlock, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    require(untilBlock <= block.number);
    bytes32 hash = keccak256(abi.encodePacked('transferFromUntil', recipient, amount, nonce, untilBlock));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
}

Backwards Compatibility

This proposal is backwards compatible for all ERC20 tokens and contracts. New tokens and contracts moving forward can implement the transferFrom functionality.

Implementation

contract ERCTransferFrom is ERC20 {
  mapping (address => mapping (address => uint256)) public nonceOf;
  
  function _transfer(address from, address recipient, uint256 amount, uint256 nonce) private returns (bool) {
    uint256 nextNonce = nonceOf[from][recipient] + 1;
    require(nonce == nextNonce);
    bool success = super._transfer(from, recipient, amount);
    if (success) nonceOf[from][recipient] = nextNonce;
    return success;
  }

  function transferFrom(address recipient, uint256 amount, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    bytes32 hash = keccak256(abi.encodePacked('transferFrom', recipient, amount, nonce));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
  }

  function transferFromUntil(address recipient, uint256 amount, uint256 untilBlock, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    require(block.number <= untilBlock);
    bytes32 hash = keccak256(abi.encodePacked('transferFromUntil', recipient, amount, nonce, untilBlock));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
  }
}
@github-actions
Copy link

Since this is your first issue, we kindly remind you to check out EIP-1 for guidance.

@zemse
Copy link
Contributor

zemse commented Oct 18, 2020

Is there any security consideration for replay protection?

@gkucmierz
Copy link
Author

Design of this function do not allow to replay attack because in case when contract is transfering tokens to itself or other contract it controls flow of code. And no other contract code is run like for example in ERC 223 or ERC 667

@zemse
Copy link
Contributor

zemse commented Oct 18, 2020

Oh, I think your reply sounds for re-entrancy explanation, pls correct me if I'm wrong. I meant to say when someone has the signature, they could simply call the function multiple times. Just in case, you can see how this is handelled by ethereum in the protocol level https://medium.com/swlh/ethereum-series-understanding-nonce-3858194b39bf.

@gkucmierz
Copy link
Author

gkucmierz commented Oct 18, 2020

You are correct, it is not considered here, but it should be.

I have an idea how to solve this problem, but there may be better solution.

Basic idea is to keep in token contract all hashes that was already executed.

mapping (bytes32 => bool) transferred;
  
function transferFrom(address recipient, uint256 amount, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    bytes32 hash = keccak256(abi.encodePacked('transferFrom', recipient, amount));
    require(!transferred[hash]);
    address from = ecrecover(hash, _v, _r, _s);
    bool success = super._transfer(from, recipient, amount);
    transferred[hash] = success;
    return success;
}

@MicahZoltu
Copy link
Contributor

That would prevent the sender from sending the same amount of tokens to the same recipient twice in a row.

Consider adding a nonce variable instead to handle replay attacks in a similar way as to how Ethereum handles them.

@gkucmierz
Copy link
Author

gkucmierz commented Oct 19, 2020

I fixed this by adding additional nonce to hash function.

contract ERCTransferFrom is ERC20 {
  mapping (bytes32 => bool) transferred;
  
  function _transfer(address recipient, uint256 amount, bytes32 hash, uint8 _v, bytes32 _r, bytes32 _s) private returns (bool) {
    require(!transferred[hash]);
    address from = ecrecover(hash, _v, _r, _s);
    bool success = super._transfer(from, recipient, amount);
    transferred[hash] = success;
    return success;
  }

  function transferFrom(address recipient, uint256 amount, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    bytes32 hash = keccak256(abi.encodePacked('transferFrom', recipient, amount, nonce));
    return _transfer(recipient, amount, hash, _v, _r, _s);
  }

  function transferFromUntil(address recipient, uint256 amount, uint256 untilBlock, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    require(block.number <= untilBlock);
    bytes32 hash = keccak256(abi.encodePacked('transferFromUntil', recipient, amount, untilBlock, nonce));
    return _transfer(recipient, amount, hash, _v, _r, _s);
  }
}

This nonce needs to be handled outside of contract.

@MicahZoltu
Copy link
Contributor

If someone decides to move forward with this as an EIP PR, I recommend adjusting the specification to only specify the interface and define what the parameters represent. You can move the implementation into an ## Implementations section if you think a reference implementation adds additional value.

@gkucmierz
Copy link
Author

gkucmierz commented Oct 19, 2020

I removed transferred hashmap as you suggested above.

contract ERCTransferFrom is ERC20 {
  mapping (address => mapping (address => uint256)) public nonceOf;
  
  function _transfer(address from, address recipient, uint256 amount, uint256 nonce) private returns (bool) {
    uint256 nextNonce = nonceOf[from][recipient] + 1;
    require(nonce == nextNonce);
    bool success = super._transfer(from, recipient, amount);
    if (success) nonceOf[from][recipient] = nextNonce;
    return success;
  }

  function transferFrom(address recipient, uint256 amount, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    bytes32 hash = keccak256(abi.encodePacked('transferFrom', recipient, amount, nonce));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
  }

  function transferFromUntil(address recipient, uint256 amount, uint256 untilBlock, uint256 nonce, uint8 _v, bytes32 _r, bytes32 _s) public returns (bool) {
    require(block.number <= untilBlock);
    bytes32 hash = keccak256(abi.encodePacked('transferFromUntil', recipient, amount, nonce, untilBlock));
    address from = ecrecover(hash, _v, _r, _s);
    return _transfer(from, recipient, amount, nonce);
  }
}

In this implementation nonce handles replay-attack and also allow make transactions with the same amount and recipient.

@zemse
Copy link
Contributor

zemse commented Oct 31, 2020

I have a suggestion to rename it into transferBySig. This way, the contract stays ERC20, along with an added functionality. Also to classify between transferBySig and transferFromBySig, as transferFromBySig requires approve done. Though both could be added.

In this ERC20 extension, you could simply add these 4 methods into the ERC3055 interface:

  • transferBySig
  • transferFromBySig
  • transferBySigUntil
  • transferFromBySigUntil

Edit: It's true that this transferFrom would have a different signature, but I think it would be preferable to have a different reasonable name if possible because it would prevent using bracket syntax in the JS side since it doesn't support overloading. Also given that, this method will probably only called from client side instead of contract to contract message calls.

// when same name methods are present
contractInstance['transferFrom(address,address,uint256)'](...args)
contractInstance['transferFrom(address,uint256,uint256,uint8,bytes32,bytes32)'](...args)

// vs when all methods in contract are different
contractInstance.transferBySig(...args)

@gkucmierz
Copy link
Author

gkucmierz commented Nov 1, 2020

@zemse I will consider changing names.

Actually we are implementing this in Wisdom token.

Here is our implementation: https://github.com/Experty/wisdom-contract/blob/ce7a5b35792279551e6b5518e4ef66e219a4bb6a/src/ERCTransferFrom.sol

That implementation also utilizes EIP #712 https://eips.ethereum.org/EIPS/eip-712 which is making this more secure.

@gkucmierz
Copy link
Author

gkucmierz commented Nov 1, 2020

@zemse can you explain difference between transferBySig and transferFromBySig?
If I understand correctly transferFromBySig is the same as transferFrom from my implementation right?
If so, what transferBySig should do?

Also transferFrom can be dropped completely, because actually its behaviour can be mimic by using transferFromUntil with very big block.number.

@zemse
Copy link
Contributor

zemse commented Nov 1, 2020

By transferFromBySig I mean the one who is giving signature has an allowance from the from address. Just like the original transferForm

Also transferFrom can be dropped completely, because actually its behaviour can be mimic by using transferFromUntil with very big block.number.

I see, that makes sense. This would reduce the functions

@gkucmierz
Copy link
Author

I am not sure If I understand this.
Intention of this proposal is to not require any allowances.
Allowance here can be signed off-chain.
I see potential that additional function can be implemented which is pure and view and can create signature. But this process can be done also off-chain in any other language.

@github-actions
Copy link

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Oct 24, 2021
@github-actions
Copy link

github-actions bot commented Nov 7, 2021

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this as completed Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants