Invoke contracts' payable method on asset Transfers

[shargon]

Related to https://cryptoslate.com/this-user-lost-1m-of-top-defi-coin-aave-aave-by-accident-heres-why/
Close neo-project/proposals#108

[roman-khimov]

neo-project/proposals#108 is good one actually, it can simplify things for contracts like NeoFS mainnet contract and the one from #1573 where some deposit is being made, it allows to make it in the most natural way, by transferring tokens to the contract address.

[roman-khimov]

This in part is #1879, but I don't think we can be that aggressive here without #1883 and checking that the contract in question is NEP-5 compliant in the first place. Otherwise we're reserving this name for NEP-5 contracts only (and there is #1646 for the name also).

[roman-khimov]

One can say that this should be checked by the contract. Though if we're to make sure this is a valid transfer event, maybe it's worth enforcing it here.

[roman-khimov]

BTW, this can also prevent some unexpected transfers, like NeoFS contract is happy to accept some GAS, but if you're to throw any NEO at it --- it doesn't know how to handle that, so the contract can check what is being transferred to it and reject things it doesn't expect.

[shargon]

Yes, you can filter with your own code.

[Tommo-L]

May happen re-entrancy attack?

 Nep5A.Transfer -> `_onPaymentReceived` -> NepA.Transfer .......?  

Then the user's asset can be transfered

[shargon]

Of course, the users need to be aware and use invocation counter or decrease the balance before send the notification

[erikzhang]

I think that if necessary, it should be called by the contract, not by the system.

[roman-khimov]

0. IIUC you're talking about something like this in transaction:

PUSH 100
PUSH CONTRACT_HASH
PUSH USER_ADDRESS
PACK 3
PUSH "transfer"
PUSH ASSET_HASH
SYSCALL System.Contract.Call
PUSH "_onPaymentReceived"
PUSH CONTRACT_HASH
SYSCALL System.Contract.CallIfExists

If so, then

1. It's a different level of guarantees. This (Invoke contracts' payable method on asset Transfers #2012) implementation can prevent token loss and can also guarantee proper handling of incoming (to the contract's address) assets. Making it transaction's script responsibility breaks that.
2. If it's to be done with a separate method the contract will then have to check for notifications and implement transfer verification/parsing of its own. It's error prone and I'd rather have correct data in the contract provided by the system.

[erikzhang]

I mean, the NEP-5 contract is obliged to call the _onPaymentReceived method of the target address after the transfer. This should become part of the NEP-5 standard.

[shargon]

I mean, the NEP-5 contract is obliged to call the _onPaymentReceived method of the target address after the transfer. This should become part of the NEP-5 standard.

Then, we need to remove the '_' because it will be called by the contract, and it can be called manually so it's untrustable, you need to check the notifications.

[roman-khimov]

Yeah, contract-driven invocation is problematic too: contracts can skip it, it can be invoked from somewhere else, receiver has to check for notifications. Better have it done by the system.

[erikzhang]

You don't need to read the notifications. The caller is the asset id. The value can be passed as a parameter. And you must trust the NEP-5 contract, because the notification is sent by the caller too.

[roman-khimov]

But it's still a different level of guarantees. Contract can just not invoke the method while the system can be trusted to always do that.

You're probably concerned with notification parsing required for this, but I think we need it anyway for #1879. And performance-wise that's what we're always doing in neo-go by default (batteries included, no plugins for us), it is visible in the profiler, but I think it's affordable.

[shargon]

You don't need to read the notifications. The caller is the asset id. The value can be passed as a parameter. And you must trust the NEP-5 contract, because the notification is sent by the caller too.

That it's true, and usually this method only will check the caller and throw an exception, if you want to do more logic you can check the notifications

[roman-khimov]

There is another problem with non-system invocations: manifested contract's permissions/trusts. The call might get blocked and at the same time we can't predict in the NEP-5 contract what other contracts can be called on transfer, so with this scheme all NEP-5 will need to have "*/*" permissions which makes permission system not very useful. Invocation by the system doesn't have this problem.

[erikzhang]

Indeed. But the actual use of permissions is actually for dynamic sharding. But if the system calls the target contract, this will also destroy the sharding.

[roman-khimov]

Yeah, that's true (hi, #546). And I don't see any simple solution for that. Callback execution could probably be moved into some other post-transaction-execution context that would be somehow tied to the transaction execution and could affect it (make it fail if it fails), but that would be very complicated and fragile.

[roman-khimov]

I don't see any simple solution for that.

Or maybe there is one. At least allowing for #546 and not considering real storage sharding (maybe NeoFS is a better example of how storage sharding is to be done when needed).

One thing we're currently missing for #546 is manifests for entry scripts, they're free to call anything and it's a problem. But instead of adding full-blown manifests (or just permissions) to them (that would most of the time contain just one entry for the contract being called by the script) we can provide a list of contracts allowed to execute within the context of this transaction (not exactly an "execution plan", but something reminiscent). This way we can leave permissions/trusts for security purposes and let execution parallelization be solved by using these lists. And these lists are easily obtained during test invocations that won't be restricted.

I don't expect these lists to be big (and again, to tackle #546 some additional in-transaction data is needed anyway), so it shouldn't be a problem from size perspective. And it all can be done as an experimental attribute for the start. So we can use system-induced invocations for onPayment feature and allowed (used) contract sets for #546.

[erikzhang]

so with this scheme all NEP-5 will need to have "/" permissions which makes permission system not very useful.

In fact they only need */onPayment.

[Tommo-L]

For me, only NEO/GAS can invoke _onPaymentReceived is enough when transfer, don't put it into NEP5 standards. For other NEP5s, they can implement it in contract level if they want. And we can't implement all the NEPs about contract.

[Tommo-L]

NEP11 about NFT, also has Transfer method, maybe there will be more NEP-X has Transfer method, too. I'm not sure they all have _onPaymentReceived requirement.

[roman-khimov]

only NEO/GAS can invoke _onPaymentReceived is enough when transfer

Well, we can get back a little to https://cryptoslate.com/this-user-lost-1m-of-top-defi-coin-aave-aave-by-accident-heres-why/ that triggered this PR. Does it solve this problem? Should we ignore this problem?

NEP11 about NFT, also has Transfer method

That's exactly why we need #1883 (with #1879, of course), this logic should only be used for verified NEP-5 and NEP-11 contracts. And other contracts can emit any kind of "transfer" events they want.

[Tommo-L]

Does it solve this problem? Should we ignore this problem?

There some other ways to solve it.

1. implement the verify method in contract.
2. make a strandard in NEP5, call onPaymentReceived in Transfer method.
3. make the contract under DAO or multisign control.
4. ...

[erikzhang]

make a strandard in NEP5, call onPaymentReceived in Transfer method.

That's what we are doing in neo-project/proposals#124.

[shargon]

Closed in favor of #2024