-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Comments
Since this is your first issue, we kindly remind you to check out EIP-1 for guidance. |
Is there any security consideration for replay protection? |
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 |
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. |
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;
} |
That would prevent the sender from sending the same amount of tokens to the same recipient twice in a row. Consider adding a |
I fixed this by adding additional 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. |
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 |
I removed 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 |
I have a suggestion to rename it into In this ERC20 extension, you could simply add these 4 methods into the
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) |
@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. |
@zemse can you explain difference between Also |
By
I see, that makes sense. This would reduce the functions |
I am not sure If I understand this. |
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. |
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. |
Preamble
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 specifiedrecipient
.Abstract
This adds a new function to ERC20 token contracts,
transferFrom
* which can be called to transfer tokens from another address.transferFrom
have different signature than originalERC20
transferFrom
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
transferFrom
transfers tokens tofrom
, which is calculated usingecrecover
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
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
The text was updated successfully, but these errors were encountered: