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.

Sign up for GitHub

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

Jump to bottom

Invoke contracts' payable method on asset Transfers #2012

Closed
wants to merge 6 commits into from
Closed

Invoke contracts' payable method on asset Transfers #2012

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ebddaf
React token transfers
shargon Oct 21, 2020
6fe120a
Change to all
shargon Oct 21, 2020
58a7cba
Remove NEP5 changes because is not payable
shargon Oct 21, 2020
a45d7ef
Clean code
shargon Oct 21, 2020
2e18d11
Rename
shargon Oct 22, 2020
739aff3
Merge branch 'master' into transfer-react
shargon Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/neo/SmartContract/ApplicationEngine.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Neo.Cryptography.ECC;
using Neo.IO;
using Neo.Network.P2P.Payloads;
using Neo.SmartContract.Manifest;
using Neo.SmartContract.Native;
using Neo.SmartContract.Native.Oracle;
using Neo.VM.Types;
Expand Down Expand Up @@ -187,6 +188,39 @@ protected internal void SendNotification(UInt160 hash, string eventName, Array s
Notify?.Invoke(this, notification);
notifications ??= new List<NotifyEventArgs>();
notifications.Add(notification);

// React to transfer event

if (eventName == "Transfer")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
if (state.Count < 3) throw new ArgumentException("Wrong Transfer definition");

var to = state[1].GetSpan();
if (to.Length != UInt160.Length) throw new ArgumentException("Wrong Transfer definition");

var contract = Snapshot.Contracts.TryGet(new UInt160(to));
if (contract == null) return;
if (!contract.Payable) throw new InvalidOperationException("Not payable contract");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Validate

var method = contract.Manifest.Abi.GetMethod("_onPaymentReceived");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May happen re-entrancy attack?

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

Then the user's asset can be transfered

Copy link
Member Author

@shargon shargon Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (method == null) return;

ContractManifest currentManifest = Snapshot.Contracts.TryGet(CurrentScriptHash)?.Manifest;
if (currentManifest != null && !currentManifest.CanCall(contract.Manifest, method.Name))
throw new InvalidOperationException($"Cannot Call Method {method} Of Contract {contract.ScriptHash} From Contract {CurrentScriptHash}");

// Call _fallback method

CallContractInternal(contract, method, new Array(ReferenceCounter)
Copy link
Contributor

@roman-khimov roman-khimov Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can filter with your own code.

{
CurrentScriptHash.ToArray() /* Token */,
state[0].GetSpan().ToArray() /* From */,
state[2].GetInteger() /* Amount */
},
CallFlags.All, CheckReturnType.EnsureIsEmpty);
}
}

protected internal NotifyEventArgs[] GetNotifications(UInt160 hash)
Expand Down