Change transferFrom method to use an originator

[jeisses]

I think there is a flaw in the logic of transferFrom. According to NEP5 the TransferFrom method should have an argument "originator" to allow spending on behalf of an "owner" to any address. It also requires a CheckWitness on the originator (along with the existing check on the allowance). See also #17

This commit includes the changes to accomplish this; I did not thoroughly test it yet

[brianlenz]

The idea of NEP-5 is to essentially replicate ERC20. I would argue that in this case, the NEP-5 proposal should be updated so that it is consistent with ERC20. I would agree that the description is inconsistent with the intent:

Syntax: public static bool transferFrom(byte[] originator, byte[] from, byte[] to, BigInteger amount) Remarks: "transferFrom" will transfer an amount from the from account to the to acount if the originator has been approved to transfer the requested amount.

The originator is actually supposed to be inferred, not passed as an argument (which would make this method more complex than it needs to be).

Here's the EIP for ERC20's transferFrom:

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md#transferfrom

The intent is to not limit who can complete the transfer so that another smart contract (for example) could complete the transfer on behalf of someone else.

Thus, I don't believe there is any need to call CheckWitness in the NEP-5 transferFrom method, and therefore the implementation in the neo-ico-template should remain as-is.

[SilverDragon135]

From technical point of view, it may be good idea to do this. But I think, we need some way to prevent abuse.

(Not sure, but maybe it would be possible to CheckWitness only if user is executing the contract and let the contracts do their jobs. Since price for deploying SC is relatively high and this type of attack won't provide any profit)

I know, that we have in theory throughput of 10000 tx/s. But if we keep transferFrom like this, I can call per each 1 approved unit of asset (for assets with 8 decimal places) 100M transferFrom.

And in scenario where we have working exchanges and 1000s approved assets, I can keep network suffer for a while. I know, there is some way how to prioritize txs based on NEO ID. Are we willing to leave it to chance?

We should move this issue to NEP proposals probably and find solution, how to prevent rogue transfers and network overload by them.

[jeisses]

Hi,

I agree that the function of transferFrom should stay similar to the ERC20 standard. However, I still think it's necessary to have an "originator" pattern to achieve this. In order to allow a contract to transer on your behalf there should be a check that only this originator can execute the method. Especially as there should be no restriction on the to address

I'm not sure if you can infer the originator value, just like we have to pass the from address to the transfer function

[brianlenz]

I know, that we have in theory throughput of 10000 tx/s. But if we keep transferFrom like this, I can call per each 1 approved unit of asset (for assets with 8 decimal places) 100M transferFrom.

You won't reliably have invocations process unless you pay a GAS fee for each. Since you'd be paying to do this kind of attack, I think that eliminates it as a good attack vector. If DoS-style attacks are the only concern, there are a myriad of other ways to do similar tx overloads aside from just transferFrom.

@jeisses, your approach may have some valid uses cases, but that's not how ERC20 works. To me, that seems like even more rare of a use case, due to added complexity. In your model, you'd have to approve the transfer of your funds to another address, but only if a third address performs the transfer.

I'm not a decision-maker, though 😁 Just offering my thoughts here. I'd recommend suggesting your idea at https://github.com/neo-project/proposals and see what comes of it 👍 One way or another, I do think the NEP-5 proposal needs to be updated...

[SilverDragon135]

I though under 10 GAS are transactions free and this way it could be done without any assets needed.

Nvm, we should stick with the topic. I think in the context of standards, the nex ico template does not conform to NEP5 standard. It may conform to ERC20, but NEP5 is clear in transformFrom.

So I do not see reason why we (participants in this issue) should create issue in Neo proposals repository. Either the NEX should accept the NEP5 or create issue in Neo proposals with request for NEP5 change. Ideally firstly pull to conform NEP5 and then solve the NEP5 problems.

@brianlenz I do not see reason, why should smartcontract complete the transfer on behalf of someone else. This can be achieved anyway, just not trough transferFrom. So I do not believe, it was intended to conform with ERC20, same as I do not believe, that in smart economy should have anything control over my transfer without explicit permission.

[localhuman]

This is NEP5 https://github.com/neo-project/proposals/blob/master/nep-5.mediawiki

transferFrom (optional)

• Syntax: public static bool transferFrom(byte[] originator, byte[] from, byte[] to, BigInteger amount)
• Remarks: "transferFrom" will transfer an amount from the from account to the to account if the originator has been approved to transfer the requested amount.

I'm not sure how this implementation doesn't conform with that.

NEP5.1 (https://github.com/neo-project/proposals/pull/18) is an extended proposal which has not been approved yet. That is a much different topic.

I can think of many use cases where it would be nice to have another entity perform the transfer of tokens from the from address to the to address on the user's behalf.

[SilverDragon135]

@localhuman I see many use cases too, what I wanted to say is, that there is no reason to use transferFrom for that. I see NEP5 as standard for basic token functionality, not as banking system 😃 Seems like I don't understand that properly, sorry for that 😞

[jeisses]

Ideally firstly pull to conform NEP5 and then solve the NEP5 problems

I agree with this

@localhuman for the contract to conform with the mentioned (optional) NEP5 method it should at least implement the method signature

An other issue with not having a checkWitness in transferFrom is that anybody could watch the blockchain for approve events and immediately execute the transferFrom which nullifies the feature

[laurensV]

Actually, the NEP5 standard is in line with how ERC20 works, and this template is not. In this template, the spender in the approve function is the same address as the to. This means that the allowance you set is specific for 1 receiver. In both the NEP5 protocol and the ERC20 protocol the allowance can have any receiver, not just 1. The approved address in the approve function should be the same as the originator (the one that is invoking the transferFrom function and that should have a checkWitness check) in the transferFrom function.

In ERC20 they don't pass the originator as an argument, because its retrieved from the address that is invoking the smart contract. In NEO, you have to pass it as an argument. That's also why the transfer function in NEP5 takes three arguments (from, to, value), while in ERC20 it only two arguments (to, value). From is retrieved from the address that is invoking the smart contract, just what they do with the originator in transferFrom.

The way that this template implements transferFrom makes no sense to me, because anybody could watch the blockchain for approve events and immediately execute the transferFrom which nullifies the feature, because then it's the same as just doing a transfer.

[conscott]

The optional methods approve, transferFrom, and allowance are currently removed from the NEP-5 standard, but I see there have been other proposals that re-introduce these.

Is the intent to still keep these methods in this contract? Seems there are some valid concerns with the current implementation - as other have noted it would be easy just to execute these approvals on the behalf of others, and also spam the network with tiny amounts from a pool of approved transfers.