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

ERC865: Pay transfers in tokens instead of gas, in one transaction #865

Open
lgalabru opened this Issue Feb 1, 2018 · 78 comments

Comments

Projects
None yet
@lgalabru
Copy link

lgalabru commented Feb 1, 2018

Preamble

EIP: <to be assigned>
Title: Token standard
Author: Ludovic Galabru
Type: Informational
Category: ERC
Status: Draft
Created: 2018-01-30
Requires: EIP20

Simple Summary

Ability for token holders to pay transfer transactions in tokens instead of gas, in one transaction.

Abstract

The following describes one standard function a token contract can implement to allow a user to delegate transfer of tokens to a third party. The third party pays for the gas, and takes a fee in tokens.

Motivation

When it comes to using tokens as utility tokens, we need to strive for a good UX. Introducing wallets and transactions to end users is a challenge, and having to explain that token holders needs ETH to send tokens is adding some friction to the process. The goal of this EIP is to abstract the gas for the end user, by introducing a fee paid in tokens. A third party can then bring the transaction on-chain, pay for the gas of that given transaction and get tokens from that user.

Specification

  • A: Sender of the payment
  • B: Recipient of the payment
  • D: Delegate, doing the transaction for A, and paying for the gas.
  • X: Amount of Token T sent from A to B
  • Y: Fee paid in Token T, from A to D for the transaction
  • T: Token to send
  • N: Nonce

Process

The user A gets a quote from the delegate D for the value of the fee Y for 1 transaction (depending on gas price + value of token in ETH).

With their private key, the user generates {V,R,S} for the sha3 of the payload P {N,A,B,D,X,Y,T}.

The user sends {V,R,S} and P (unhashed, unsigned) to the delegate.

The delegate verifies that Y and D have not been altered.

The delegate proceeds to submit the transaction from his account D:

T.delegatedTransfer(N,A,B,X,Y,V,R,S)

The delegatedTransfer method reconstructs the sha3 H of the payload P (where T is the address of the current contract and D is the msg.sender).

We can then call ecrecover(H, V, R, S), make sure that the result matches A, and if that’s the case, safely move X tokens from A to B and Y tokens from A to D.

The challenge mainly resides in imitating the Non-standard Packed Mode on the client side, and obtaining the exact same sha3 as the one generated on-chain.

Methods

delegatedTransfer

function transferPreSigned(
    bytes _signature,
    address _to,
    uint256 _value,
    uint256 _fee,
    uint256 _nonce
)
    public
    returns (bool);

Is called by the delegate, and performs the transfer.

Events

TransferPreSigned

event TransferPreSigned(address indexed from, address indexed to, address indexed delegate, uint256 amount, uint256 fee);

Is triggered whenever delegatedTransfer is successfully called.

Implementation proposal

Assuming a StandardToken and SafeMath available, one could come up with the following implementation.

On-chain operation (solidity)

    /**
     * @notice Submit a presigned transfer
     * @param _signature bytes The signature, issued by the owner.
     * @param _to address The address which you want to transfer to.
     * @param _value uint256 The amount of tokens to be transferred.
     * @param _fee uint256 The amount of tokens paid to msg.sender, by the owner.
     * @param _nonce uint256 Presigned transaction number. Should be unique, per user.
     */
    function transferPreSigned(
        bytes _signature,
        address _to,
        uint256 _value,
        uint256 _fee,
        uint256 _nonce
    )
        public
        returns (bool)
    {
        require(_to != address(0));

        bytes32 hashedParams = transferPreSignedHashing(address(this), _to, _value, _fee, _nonce);
        address from = recover(hashedParams, _signature);
        require(from != address(0));

        bytes32 hashedTx = keccak256(from, hashedParams);
        require(hashedTxs[hashedTx] == false);

        balances[from] = balances[from].sub(_value).sub(_fee);
        balances[_to] = balances[_to].add(_value);
        balances[msg.sender] = balances[msg.sender].add(_fee);
        hashedTxs[hashedTx] = true;

        Transfer(from, _to, _value);
        Transfer(from, msg.sender, _fee);
        TransferPreSigned(from, _to, msg.sender, _value, _fee);
        return true;
    }

    /**
     * @notice Hash (keccak256) of the payload used by transferPreSigned
     * @param _token address The address of the token.
     * @param _to address The address which you want to transfer to.
     * @param _value uint256 The amount of tokens to be transferred.
     * @param _fee uint256 The amount of tokens paid to msg.sender, by the owner.
     * @param _nonce uint256 Presigned transaction number.
     */
    function transferPreSignedHashing(
        address _token,
        address _to,
        uint256 _value,
        uint256 _fee,
        uint256 _nonce
    )
        public
        pure
        returns (bytes32)
    {
        /* "48664c16": transferPreSignedHashing(address,address,address,uint256,uint256,uint256) */
        return keccak256(bytes4(0x48664c16), _token, _to, _value, _fee, _nonce);
    }

Off-chain usage (js)

    describe(`if Charlie performs a transaction T, transfering 100 tokens from Alice to Bob (fee=10)`, () => {
      beforeEach(async () => {
        const nonce = 32;
        const from = alice;
        const to = bob;
        const delegate = charlie;
        const fee = 10;
        const amount = 100;
        const alicePrivateKey = Buffer.from('c87509a1c067bbde78beb793e6fa76530b6382a4c0241e5e4a9ec0a0f44dc0d3', 'hex');

        const components = [
          Buffer.from('48664c16', 'hex'),
          formattedAddress(this.token.address),
          formattedAddress(to),
          formattedInt(amount),
          formattedInt(fee),
          formattedInt(nonce)
        ];
        const vrs = ethUtil.ecsign(hashedTightPacked(components), alicePrivateKey);
        const sig = ethUtil.toRpcSig(vrs.v, vrs.r, vrs.s);
        await this.token.transferPreSigned(
          sig,
          to,
          amount,
          fee,
          nonce
          , {from: charlie}).should.be.fulfilled;
      });

Full implementation available

OpenZeppelin/openzeppelin-solidity#741

Additional documentation

Transfer Ethereum tokens without Ether — An ERC20 Improvement to Seriously Consider

Copyright

Copyright and related rights waived via CC0.

@lgalabru lgalabru changed the title ERC864: Pay transfers in tokens instead of gas in one transaction ERC865: Pay transfers in tokens instead of gas in one transaction Feb 1, 2018

@lgalabru lgalabru changed the title ERC865: Pay transfers in tokens instead of gas in one transaction ERC865: Pay transfers in tokens instead of gas, in one transaction Feb 1, 2018

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 2, 2018

I'm working on something simmilar for about a week now, but I've come with a different implementation.
I've realized this idea while writing this comment https://ethresear.ch/t/pos-and-economic-abstraction-stakers-would-be-able-to-accept-gas-price-in-any-erc20-token/721/7?u=3esmit as I was told it was probably not going to happen I came up with this solution status-im/swarms#73
A first version was deployed here: https://ropsten.etherscan.io/address/0x9787564e1bd7da95ee9dcdf17cc57a7225084632
And the latest version is available here https://github.com/status-im/contracts/blob/presigned-token/contracts/token/MiniMeTokenPreSigned.sol
An example of gas paid in Token transfer: https://ropsten.etherscan.io/tx/0x52a886755876e7f88fed90cae3f58ee8e00cdcaa2dac24382202d0e37ed14059

@ptrwtts

This comment has been minimized.

Copy link

ptrwtts commented Feb 2, 2018

Nice work @3esmit. Do you have any comments on the pros / cons of this implementation vs yours?

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Feb 2, 2018

3esmit pretty cool! I really like your approach on having a separate method for generating the keccak, that can be re-used with web3 for simplifying the keccak generation off-chain.
For the off chain side, my initial approach was the following:

      const fee = 10;
      const amount = 100;
      const token = this.token.address;
      const from = alice;
      const to = bob;
      const delegate = charlie;
      const nonce = 32;

      const bufferedAddress = (address) => {
        return  Buffer.from(ethUtil.stripHexPrefix(address), 'hex');
      };
      const bufferedInt = (int) => {
        return ethUtil.setLengthLeft(int, 32);
      };
      const formattedByte32 = (bytes) => {
        return ethUtil.addHexPrefix(bytes.toString('hex'));
      };

      const components = [
        bufferedAddress(delegate),
        bufferedAddress(token),
        bufferedInt(nonce),
        bufferedAddress(from),
        bufferedAddress(to),
        bufferedInt(amount),
        bufferedInt(fee)
      ];

      const tightPack = Buffer.concat(components);

      const hashedTightPack = ethUtil.sha3(tightPack);

      const alicePrivateKey = Buffer.from('c88b703fb08cbea894b6aeff5a544fb92e78a18e19814cd85da83b71f772aa6c', 'hex');

      const sig = ethUtil.ecsign(hashedTightPack, alicePrivateKey)

      const pubkey = ethUtil.ecrecover(hashedTightPack, sig.v, sig.r, sig.s)
      const address = ethUtil.publicToAddress(pubkey)

      const tx = await this.token.delegatedTransfer(
        nonce,
        from,
        to,
        amount,
        fee,
        sig.v,
        formattedByte32(sig.r),
        formattedByte32(sig.s), {from: charlie});

Pretty verbose and error prone.
Now, if we change your implementation from

    function getTransferHash(
        address _to,
        uint256 _value,
        uint256 _gasPrice,
        uint256 _nonce
    )
        constant
        public
        returns(bytes32 txHash)
    {
        //"edde766e": "transferPreSigned(uint8,bytes32,bytes32,address,uint256,uint256,uint256)",
        txHash = keccak256(address(this), bytes4(0xedde766e), _to, _value, _gasPrice, _nonce);
    }

to

    function getTransferHash(
        address _contract
        address _to,
        uint256 _value,
        uint256 _gasPrice,
        uint256 _nonce
    )
        constant
        public
        returns(bytes32 txHash)
    {
        //"12345678": "getTransferHash(address,address,uint256,uint256,uint256)",
        txHash = keccak256(bytes4(0x12345678), _contract,  _to, _value, _gasPrice, _nonce);
    }

We now have a reliable way to build the exact same keccak on chain and off chain, by simply using the ABI of the contract and directly hashing the data coming from:

token.getTransferHash.request(_contract,  _to, _value, _gasPrice, _nonce)

What do you think?

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 2, 2018

@ptrwtts My approch support arbitrary contract executions in SNT network, so we can even create a new contract and pay the gas relayed by SNT "gas". Aswell don't need quotes from delegates, the system works simillar way ETH gasPrice market works, but instead of being backed by USD value is by ETH value.

@lgalabru I'm not sure why supporting any contract address for building the txHash, initially I did so and used address(this) at it's calling, and should not be a problem. If you prefer that, I suggest setting function as pure.

@bokkypoobah live in which network? Mainnet?
I deployed an example of SNTPreSigned at Ropsten, check it out https://ropsten.etherscan.io/address/0xb473fd6c1206655bf2385013b23589f713fe3213

@bokkypoobah

This comment has been minimized.

Copy link
Contributor

bokkypoobah commented Feb 2, 2018

@3esmit BTTSTokenFactory on mainnet - https://etherscan.io/address/0x14aabc5ade82240330e5be05d8ef350661aebb8a#code and a token - https://etherscan.io/address/0x4ac00f287f36a6aad655281fe1ca6798c9cb727b#code .

And while developing it, I was getting an out of stack space for variables problem in Solidity when using r,s and v, so switched to using sig.

I also added *Check functions so the service provider is able to confirm that the transaction has not already been executed. And to get the exact error before executing the transaction, if the transaction is expected to fail.

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Feb 2, 2018

@3esmit the idea was to use the same method (indeed, pure), on-chain and off chain.
From the smart contract, you could have use:

 bytes32 hash = getTransferHash(
        address(this)
        _to,
        _value,
        _gasPrice,
        _nonce
    )

And off-chain, you could have use:

const payload = await token.getTransferHash.request(token.address,  _to, _value, _gasPrice, _nonce);
const data = payload.params[0].data;
const hash = ethUtil.sha3(data);

After testing this approach, the problem is that off chain, when building data, all the arguments are padded vs being tightly-packed on chain.
Instead of hashing:

d1d4e623d10f9fba5db95830f7d3839406c6af22932b7a2355d6fecc4b5c0b6bd44cc31df247a2e

you're hashing:

0000000000000000000000000d1d4e623d10f9fba5db95830f7d3839406c6af20000000000000000000000002932b7a2355d6fecc4b5c0b6bd44cc31df247a2e
@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 2, 2018

Fundamentally to process the contract call you need to be synced with state, the only difference is that you can call that function pure then, there is no other benefict, I would stay with being processed internally because this is a known common header of signature and to benefict of a simplier function signature.
Maybe if gets cheaper in gas by using the pure function, then would be interesting, but I don't think this will make a difference overall.

@seweso

This comment has been minimized.

Copy link

seweso commented Feb 2, 2018

Wouldn't a way simpler solution be to have contracts set gasprice & startgas so miners are allowed to take gas from the contracts balance?

Then nodes/miners would use limited gas per transaction to determine if these transactions are mineable at all (gas price is high enough) and warrant to be re-broadcast over the network. In other words, nodes/miners are executing the code of transactions (where the sender doesn't have enough funds themselves), and which contain 'gasprice & startgas'-opcodes.

Contracts can then convert tokens to gas themselves. Basically wallets would allow users to set gasprice & startgas in the used token, and the contract converts that to ETH. Or if the contract is rather predictable in gas usage make it even easier for users by allowing users to only set gasprice.

@aakilfernandes

This comment has been minimized.

Copy link

aakilfernandes commented Feb 2, 2018

Very cool. This would be extremely useful from a UX perspective (users don't have to hold multiple tokens).

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Feb 2, 2018

@seweso I think the change you're describing needs to happen on the EVM level (and is on the roadmap if my understanding is correct).
This is less trivial than just iterating on top of ERC20.

@mattdf

This comment has been minimized.

Copy link
Member

mattdf commented Feb 2, 2018

It would be good to standardize this to cover delegated ETH transfers as well when token = 0x0, for things like withdrawing from ring signature mixers. As otherwise, you would have to "pre-fund" the withdrawing account, thereby linking your deposit and withdraw addresses.

@seweso

This comment has been minimized.

Copy link

seweso commented Feb 2, 2018

@lgalabru Yes agreed. And it is a direction Ethereum needs to go into.

Do you know how this is called on the roadmap? I couldn't find it.

@ojanssens

This comment has been minimized.

Copy link

ojanssens commented Feb 2, 2018

I think this is a great idea and an answer to my problem I’ve been struggling with: https://forum.ethereum.org/discussion/16990/transaction-fees-for-an-erc20-currency

I only have 1 concern though: Wouldn’t this make Ether itself redundant? If miners would be able to directly accept tokens as payment, the utility of Ether would become less relevant except for staking and securing the network (which is still extremely valuable of course).

@Philipinho

This comment has been minimized.

Copy link

Philipinho commented Feb 3, 2018

@bokkypoobah, i'm curious to ask. What happens if the tokens are worthless? How will BTTS pay for the gas in ether when the token has no value?

@bokkypoobah

This comment has been minimized.

Copy link
Contributor

bokkypoobah commented Feb 3, 2018

@Philipinho I plan to provide a smart contract that the token contract owner has to top up with ETH, and this smart contract will buy tokens from the BTTS service provider at a specified rate.

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 3, 2018

I think that defining a gasPrice instead a fee is more safe and dynamic, there is no reason for not doing this.
It's trivial to calculate the gas cost of an operation inside the smart contract. The global msg.gas returns the remaining gas, so simply read this value at bounds of operation (right after sending the gas).

This is really important for approveAndCallPreSigned, because this function can call arbritary execution in other contract that can be hard to be estimated, so we place the responsability of being safe to signer.
gasLimit could be interesting to safety of both sides, but the limiting already exists in native gas.

Please review my work in MiniMeTokenPreSigned.sol and derive from it, it's the same GNU license followed by MiniMeToken.sol (I guess GPLv3, @jbaylina?).

The interface I suggest is the following:

pragma solidity ^0.4.17;

/**
   @notice Implements PreSigned ERC20Token operations (and approveAndCall(address,uint256,bytes);
 */
contract ERC865 {
    /**
     * @notice Include a presigned `"a9059cbb": "transfer(address,uint256)"`
     * @param _signature Signed transfer 
     * @param _to The address of the recipient
     * @param _value The value of tokens to be transferred
     * @param _gasPrice How much tokens willing to pay per gas
     * @param _nonce Presigned transaction number.
     */
    function transferPreSigned(bytes _signature, address _to, uint256 _value, uint256 _gasPrice, uint256 _nonce) public;

    /**
     * @notice Include a presigned `""095ea7b3": "approve(address,uint256)"`
     * @param _signature Signed transfer 
     * @param _to The address of the recipient
     * @param _value The value of tokens to be transferred
     * @param _gasPrice How much tokens willing to pay per gas
     * @param _nonce Presigned transaction number.
     */    
    function approvePreSigned(bytes _signature, address _to, uint256 _value, uint256 _gasPrice, uint256 _nonce) public;
    
    /**
     * @notice Include a presigned `"cae9ca51": "approveAndCall(address,uint256,bytes)"`
     * @param _signature Signed transfer 
     * @param _to The address of the recipient
     * @param _value The value of tokens to be transferred
     * @param _extraData option data to send to contract
     * @param _gasPrice How much tokens willing to pay per gas
     * @param _nonce Presigned transaction number.
     */
    function approveAndCallPreSigned(bytes _signature, address _to, uint256 _value, uint256 _extraData, uint256 _gasPrice, uint256 _nonce) public;
    
}

I'm not absolutely sure about the usage of bytes _signature instead of the offchain processed uint8 v, bytes32 r, bytes32 s. Maybe to save gas we should use it preprocessed?

Supporting additional approveAndCall is important due 2 reasons:

  1. Push forward a real solution to ERC20 aproove + 1 custom call at contract`
  2. Makes possible executing arbtirary operations on other contracts (implementing ApproveAndCallFallBack.sol), even calls with value 0 in approveAndCall (pure data calls), extended even to creation of new contracts - all gas cost relayed through gasPrice offer in contract token.

Including a fixed fee method in side of this 3 signatures would be an option, I'm not totally against it and don't see an actual problem in having this more option, or could be included as a parameter, however I think users will prefer including gasPriced transaction instead of fixedFee because its easier to calculate cost/risk (?).

PreSigned contract calls (intended to relay native gas) should also be standarized, so we can have other types of tokens which can use different call methods and wallets would recognize it.
I suggest some types of hashing we can agree on:

//"edde766e": "transfer(address,uint256)",
txHash = keccak256(address(this), bytes4(0xedde766e), keccak256(_to, _value), _gasPrice, _nonce);
txHash = keccak256(address(this), keccak256(bytes4(0xedde766e), _to, _value), _gasPrice, _nonce);
txHash = keccak256(address(this), bytes4(0xedde766e), _to, _value, _gasPrice, _nonce); //seems to be better because cheaper and no reason to tie separatedly the elements.

This way will be easier to understand what is being signed, specially for wallets that want give more details about where that hash came from.

Also the signatures should be directed towards #191 to motivate wallet developers in agreeing in the common signing method defined there.

@Arachnid

This comment has been minimized.

Copy link
Collaborator

Arachnid commented Feb 3, 2018

Excellent idea! Why explicitly encode the sender when it's provided by ecrecover, though?

Also, why encode the delegate, instead of letting the first person to get a transaction in harvest the fee?

@jdkanani

This comment has been minimized.

Copy link

jdkanani commented Feb 3, 2018

Great. @lgalabru nonce wouldn't work this way. Possible race condition - require(_nonce > nonces[_from]) prevents older transactions to go through, even if sig is valid.

Could use mapping(delegateTxHash => bool).

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Feb 3, 2018

@jdkanani
Amazing, I had the race condition in my radar and was dubitative, thanks for your workaround!

@Arachnid
Right, we need to pass it, but it doesn't have to be encoded. Good catch

Letting the first person to get a transaction in harvest the fee

Great idea!

@3esmit
Using msg.gas price makes sense, but we should probably come up with another name, since "gas price" is usually in wei.
Concerning the naming of the methods, what would you think of prefixing the methods with "delegated", instead of suffixing with "presign", for insisting on the fact that someone else is bringing the transaction on chain?
Concerning the direction, we probably want to open a pull request on Open Zeppelin (cc @spalladino). Would it work with the constraints of your licence?

@izqui

This comment has been minimized.

Copy link
Contributor

izqui commented Feb 3, 2018

I have been working on this idea for a side-project, the pay-protocol. This feature is something that can be achieved in a layer 2 protocol rather than adding this functionality to every token contract. Which also has the ability that every token currently deployed could use it, just by having people that want to use this create a ERC20 allowance to the PayProtocol contract or setting it a ERC777 operator.

In any case, I think it would be super interesting to standardize the offchain part of this, which would allow people to run nodes looking for profitable token transfers and settle them.

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 3, 2018

@lgalabru "Delegated" means that you transfered temporarily trust to other, is not the case here, so I think the naming transferPreSigned() or signedTransfer() are more correct, but transferPreSigned actually describe exacly what the function is (a transfer which been pre signed.)

I don't think is bad to use gasPrice as the name, it also describe exactly what is this parameter for, and it works just like regular native ether gasPrice, however it is under the token you're interacting.
Just like in native token, gasPrice is set in the minimum unit (wei), tokens that have decimals need to be aware of this. A good UX would handle this fine.

@izqui I think your project is a little bit different because is not the token itself that is handeling the moving, seems like is a TokenController? but should work like the same.
About the ethereum signed message, I see that you do this:
keccak256(SCHEMA_HASH, keccak256(this, token, from, to, value, expires, pull, push, keccak256(deps)));
Maybe we can have your opinion on why you did this and why we also should do something like this.

@axcrest

This comment has been minimized.

Copy link

axcrest commented Feb 4, 2018

How do you handle the ERC-20 approve() requirement? No one can withdraw funds from your token account without you first approving them, and that approve() step costs ETH.

@izqui

This comment has been minimized.

Copy link
Contributor

izqui commented Feb 4, 2018

@axcrest you are right that in pay protocol if you already have tokens you need to create an approval to the contract. But if you receive the tokens directly in your pay protocol account, then you never need ether to operate.

@3esmit you are right the token isn't handling the moving and that's why it is very efficient, token transfers are only settled in the token contracts when tokens need to move in or out of the protocol. This allows to settle MiniMe transfers for ~15k gas (if using bulk transfers, ~36k otherwise) instead of +100k that takes if operating directly on the token.

The signed message conforms to the latest #712 spec. It allows for signing providers to show exactly what is going in the hash they are going to be signing.

@3esmit

This comment has been minimized.

Copy link

3esmit commented Feb 4, 2018

@izqui Thanks for the information about signed message spec, I think the community should consensus in a default signed standard ASAP.
I liked #712 and I think it should be a subtype (version) of #191.
You are right about pay-protocol being cheaper, and it's an interesting solution, and should be used together with approveAndCallPreSigned.
The first transaction from someone entering this network could be paid in the token itself directly. I think whats being discussed here and the network you showed us are different things and compliment each other. Users always want cheaper transactions, and also may want to not hold ether.

The main issue this EIP resolves is the need of an account to hold Ether in their balance to move their token. Even being more expansive, it might be cheaper then the process of transferring ether to that account and moving it.

MiniMeTokenPresigned behaves differnt when gasPrice is zero, this can be used for an user which have 2 balances, 1 with ether and other with the token, so they can control this account using gas from other account.

I can upgrade Raiden contracts to be able of accepting approveAndCallPreSigned to open a channel, so users could participate in Raiden network without ever holding any ether. I think this is certainly an improvement for the token that reaches almost the same result as a true economic abstraction, with the restriction that contracts must be implemented ApproveAndCallFallBack.sol properly.
I'll prepare examples in the week for StandardBounty contracts and fees being paid with approveAndCallPresigned.

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Feb 7, 2018

A first implementation of this EIP is available here: https://github.com/PROPSProject/props-token-distribution/blob/master/contracts/token/ERC865Token.sol.
I'll probably open a pull request on zeppelin after getting some feedbacks from you guys.

@3esmit I'm not convinced by this idea of computing the gas cost on the fly because you're adding more complexity to the method, when you want to be really accurate.

I skipped the approveAndCall, since it's not part of the ERC20 protocol + there is no clear settlement at this point (ERC223 vs ERC827).

@bokkypoobah

This comment has been minimized.

Copy link
Contributor

bokkypoobah commented Nov 8, 2018

Here's an implementation that is live - https://github.com/bokkypoobah/BokkyPooBahsTokenTeleportationServiceSmartContract/blob/master/contracts/BTTSTokenFactory.sol#L365-L395
See https://github.com/bokkypoobah/BokkyPooBahsTokenTeleportationServiceSmartContract#how-it-works

So can I use this and modify the contract name?
It's MIT licensed, so please do.

If you don't want the hassles of deploying it yourself, there is a factory that deploys new token contracts @ https://etherscan.io/address/0x14aabc5ade82240330e5be05d8ef350661aebb8a#code . You can deploy you own token contract via the factory using a wallet GUI or some geth JavaScript console commands - see https://github.com/bokkypoobah/BokkyPooBahsTokenTeleportationServiceSmartContract#how-do-i-deploy-my-own-btts-token .

Note that this was written before ERC865 was proposed, and does not conform to the ERC865 standard (but works similarly).

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 8, 2018

@bokkypoobah Thanks very much.In order for me to learn the details I have decided to go the "deploy it yourself" route. I am reading the ERC865 version and trying to figure out how to go about it. I will let you (and anyone interested) know about my progress or if I encounter any issues.

Thanks again.

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 9, 2018

@bokkypoobah I was going through https://bit.ly/2DrJGo5 and you mentioned:

The token owner 0xa33a then uses their private key to sign the hash computed in the previous step using the function var sig = web3.eth.sign(from, signedTransferHash);

Where's the private key? I expected the 1st param to be the initiator's private key. No?

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Nov 9, 2018

@tochie if you pre-sign your transfer using web3, you don't have to pass the private key, you just need to make sure that the account you want to sign with is unlocked (
https://web3js.readthedocs.io/en/1.0/web3-eth.html#sign)

@bokkypoobah

This comment has been minimized.

Copy link
Contributor

bokkypoobah commented Nov 9, 2018

@bokkypoobah I was going through https://bit.ly/2DrJGo5 and you mentioned:

The token owner 0xa33a then uses their private key to sign the hash computed in the previous step using the function var sig = web3.eth.sign(from, signedTransferHash);

Where's the private key? I expected the 1st param to be the initiator's private key. No?

The from parameter will contain the public key. If you are using geth, you have to unlock your account using personal.unlockAccount(fromPublicKey, "password") . If you then call var sig = web3.eth.sign(from, signedTransferHash);, geth will sign your data using the unlocked from account private key

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 11, 2018

Thanks for the feedback.

@bokkypoobah @lgalbru I am not able to get past here:

var signedTransferHash = token.signedTransferHash(from, to, tokens, fee, nonce);
I get a TypeError: token.signedTransferHash is not a function. In my implementation, token was set from new web3.eth.Contract(ABI, tokenContractAddress) where the tokenContractAddress variable is the address of the token I generated from the BTTTokenFactory. I also console logged token, and couldn't find signedTransferHash. Please advise. Thanks.

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 11, 2018

After a careful research guys, I realized that those functions where shoveled under methods. 👍

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 11, 2018

@bokkypoobah, @lgalabru

Now here's my real roadblock:

Error: Invalid number of parameters for "signedTransfer". Got 8 expected 7!

The tutorial from https://bit.ly/2DrJGo5 has this test case:

var signedTransfer1Tx = token.signedTransfer(from, to, tokens, fee, nonce, sig, feeAccount, {from: contractOwnerAccount, gas: 200000, gasPrice: defaultGasPrice}); which has 8 params. How were you able to succeed here.

Please advise. Thanks.

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Nov 12, 2018

@tochie That call has 7 parameters then the default transaction parameters. The only parameters for the function are:
from, to, tokens, fee, nonce, sig, feeAccount
Check to make sure your formatting is correct and there are no parameters other than those.

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 12, 2018

@RobertMCForster Yes. I know there are 7 params but the examples from the tutorials and authors notes passed in 8 param [the default 7 params and an extra object that captures the contract owners address].

@tochie

This comment has been minimized.

Copy link

tochie commented Nov 17, 2018

Anyone to answer me? My entire application is dependent on this. For now, its a massive roadblock :)

@davis12529 davis12529 referenced this issue Dec 8, 2018

Closed

Backend API #1

@AFDudley

This comment has been minimized.

Copy link

AFDudley commented Dec 19, 2018

The fact that no one has mentioned #28 in this thread is somewhat alarming. 🙂

@tbocek

This comment has been minimized.

Copy link

tbocek commented Jan 23, 2019

There is a signature malleability issue in the full implemention mentioned on top, see OpenZeppelin/openzeppelin-solidity#1622

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Jan 24, 2019

There is a signature malleability issue in the full implemention mentioned on top, see OpenZeppelin/openzeppelin-solidity#1622

This times a million. Coinvest had to relaunch our token because we used this method. The correct way to track unique transactions is by transaction hash--NOT signature.

The problem is that signature malleability was fixed on Ethereum itself but not the ecrecover pre-compiled contract.

@lgalabru I'd highly recommend editing this to use transaction hash on the original post.

@ptrwtts

This comment has been minimized.

Copy link

ptrwtts commented Jan 29, 2019

Hi @RobertMCForster, thanks for sharing your solution.

What are your thoughts on what @tbocek proposed, where you only allow 27/28 signatures, and force clients to use them: tbocek/openzeppelin-solidity@dc168f3

With this, it should still be safe to use the signature for uniqueness. One advantage is that you don't need to use incrementing nonces and store them on chain, which complicates signature generation (need to lookup the last nonce) and costs more gas. You can just use something random (e.g. current timestamp) as the nonce.

The downside of this approach seems to be that not all clients will be compatible, if they don't adhere to only generating 27/28 signatures.

Let me know if I'm missing something.

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Jan 29, 2019

@ptrwtts , @tbocek 's solution is still vulnerable to signature malleability.

I actually missed that his post didn't mention the other, sneakier, malleability vulnerability that also comes from this method of using signatures as unique identifiers.

The problem is that with ECDSA each signature has a complementary signature that will also recover correctly.

Per Vitalik:

Allowing transactions with any s value with 0 < s < secp256k1n, as is currently the case, opens a transaction malleability concern, as one can take any transaction, flip the s value from s to secp256k1n - s, flip the v value (27 -> 28 [or] 28 -> 27), and the resulting signature would still be valid.

That quote is taken from the Homestead hardfork EIP here, which also explains that they left the validity of the top half of the elliptic curve in the ecrecover pre-compiled contract in order to maintain backwards compatibility with old Bitcoin signatures.

Now, you could restrict it even further to allow, say, only 27, but at that point you're just complicating things for no reason. The transaction hash (maybe it's better termed "parameter hash" to differentiate it from an Ethereum transaction hash) doesn't need incrementing nonces either as long as nonce is unique, it's inherently compatible with all clients, and it's just a much safer/cleaner way to do things.

P.S. In our code we left nonce incrementing in but do not require that nonce correlates to the last nonce + 1. It's meant to essentially be a "suggestion," but if I was going to rewrite it I think I'd leave that out in favor of just using timestamps like you suggested (we do this in our off-chain code anyway).

@lgalabru

This comment has been minimized.

Copy link
Author

lgalabru commented Jan 30, 2019

@RobertMCForster @tbocek @ptrwtts thank you guys for the heads up!
What's your take on the edit?

@tbocek

This comment has been minimized.

Copy link

tbocek commented Jan 31, 2019

@RobertMCForster my solution is not vulnerable to signature malleability. The spec says in Appendix F, that s is only valid with values 0< s <secp256k1n÷2 + 1 (281). Maybe are referring to an old version. The spec also says that only 27/28 is allowed (282). In EIP-2 chapter specification, point 2, they did not left the top half for backwards compatibility:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid.

By restricting to 27/28 you make it according to the specification, and fix the signature malleability. Otherwise, ERC865 as it is currently implemented can be exploited and can even potentially loose coins or tokens.

@tbocek

This comment has been minimized.

Copy link

tbocek commented Jan 31, 2019

@ptrwtts I think the nonce adds little value to the security. Or maybe I missed the reason why this nonce is important in the current implementation.

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Jan 31, 2019

@tbocek The fixes you're referring to were only applied on the Ethereum network itself, not the ecrecover pre-compiled contract.

From that EIP-2 link:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures. [bold mine]

To test it yourself, sign some data, take the s value, flip it with this: s2 = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1;, then flip the v from 27 -> 28 or vice versa. You'll see that both signatures recover correctly on-chain.

ERC865 was able to be exploited and could lose users coins. We actually had to hardfork our token because we used signatures as unique identifiers and, while we caught the 0/1 or 27/28 vulnerability before launch, we missed the complementary signature problem.

@AusIV

This comment has been minimized.

Copy link

AusIV commented Jan 31, 2019

I believe that in the current implementation, the nonce simply allows you to send the same quantity of the same token to the same recipient multiple times. Absent the nonce, you would have the same message and produce the same signature.

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Jan 31, 2019

@AusIV How would it allow this? The contract requires the hashedTx has not been used then declares it used. If the nonce and values are the same twice the transaction will revert on require(hashedTxs[hashedTx] == false); I'm dumb, that's correct.

@lgalabru Looks safe to me! One thing I'd recommend is for tx.origin to be given the fee rather than msg.sender as there may be a situation where those are not the same.

Another thing I'd be curious about getting standardized is gas price vs. gas fee. In @3esmit 's implementation he has delegates submit a gas price which is then calculated into the fee to be given to the delegate. In the current specification a delegate submits a previously calculated fee.

The tricky thing is I like gas fee more than gas price for functions like the transfer above because of gas efficiency, but when it comes to approveAndCallPreSigned (which can be extremely beneficial to ERC865 tokens), implementing a hard gas fee could result in all sorts of tomfoolery.

There are some other things that come about when more functions are added (such as a universal getPreSignedHash instead of one for each function) that I'll post more about soon with a full example.

@tbocek

This comment has been minimized.

Copy link

tbocek commented Jan 31, 2019

@RobertMCForster you are right. Thanks for clarification!

@tbocek

This comment has been minimized.

Copy link

tbocek commented Jan 31, 2019

@RobertMCForster What was your solution. Did you normalize the signature, something like:

if(s > halfOrder) {
  s = uint256(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141) - s;
}
bytes memory normalizedSignature = abi.encode(r, s);

Then it can be check for the normalizedSignature, which is unique.

Edit: I'm even considering to simply add a check (besides 0/1) to allow only signatures with a low order in Zeppelin's recover() function.

@RobertMCForster

This comment has been minimized.

Copy link

RobertMCForster commented Feb 4, 2019

@RobertMCForster What was your solution. Did you normalize the signature, something like:

if(s > halfOrder) {
  s = uint256(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141) - s;
}
bytes memory normalizedSignature = abi.encode(r, s);

Then it can be check for the normalizedSignature, which is unique.

Edit: I'm even considering to simply add a check (besides 0/1) to allow only signatures with a low order in Zeppelin's recover() function.

@tbocek My full solution is here. I used the hash of given parameters (including a random nonce) to keep track of unique transactions.

Disclaimers regarding the code:

  1. If written again I would remove the nonce mapping, nonce++, and getNonce() since, as @ptrwtts mentioned, a random value such as timestamp works fine (we're just using timestamp anyway on our off-chain code for simplicity).
  2. If you want the receiveApproval function to allow other ERC865's to send your tokens and pay their own gas, make sure you require msg.sender isn't address(this) or people can take stuck tokens from it. Oops.
  3. There are some random little efficiencies that could be done like not nesting transfer balance requires--I was paranoid from the bug on our first ERC865 version--, making gas cost calculations more accurate, and maybe do something like add amount to receiver, calculate gas, subtract total (amount+fee) from sender, add fee to delegate, instead of a full transfer to receiver then a full transfer to delegate (obviously not on approvals).

Overall, 10k+ gas on normal transfers and 5k+ on approvals could be saved from my solution and gas cost could be more accurate.

@villesundell

This comment has been minimized.

Copy link

villesundell commented Mar 12, 2019

Is it intentional, that r, s and v are in "non-standard" order in the reference implementation:
https://github.com/OpenZeppelin/openzeppelin-solidity/pull/741/files#r264813474 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.