SmartContract: use executing contract state to check permissions

[AnnaShaleva]

Description

It's not correct to use an updated contract state got from native Management to check for the allowed method call. We need to use manifest from the currently executing context for that. It may be critical for cases when executing contract is being updated firstly, and after that it calls another contract. So we need an old (executing) contract manifest for this check.

This change likely does not affect the mainnet's state since it's hard to meet the trigger criteria, thus we suggest not to use a hardfok.

A port of nspcc-dev/neo-go#3473. This bug was discovered during the similar problem fix in nspcc-dev/neo-go#3471 and nspcc-dev/neo-go#3472. I've checked all other similar usages and the rest of them use proper contract state (executing one, not the Management's one).

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• TestSystem_Contract_Call_Permissions unit test
• In progress: check mainnet states for compatibility Moved under D hardfork.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[Jim8y]

It's not correct to use an updated contract state got from native Management to check for the allowed method call. We need to use manifest from the currently executing context for that. It may be critical for cases when executing contract is being updated firstly, and after that it calls another contract. So we need an old (executing) contract manifest for this check.

Isn't this by design? I mean, it also makes sense to me to use the new state right after the its updated. Can you explain a little bit more about what problem can this cause that makes it a bug?

[AnnaShaleva]

Isn't this by design?

I doubt so. This is a quite severe bug that may lead to unwanted consequences. Besides, all other similar places use executing contract state, for example:

neo/src/Neo/SmartContract/ApplicationEngine.cs

Line 210 in fd1edf0

protected static void OnCallT(ExecutionEngine engine, Instruction instruction)

neo/src/Neo/SmartContract/ApplicationEngine.Runtime.cs

Line 349 in fd1edf0

protected internal void RuntimeNotify(byte[] eventName, Array state)

Can you explain a little bit more about what problem can this cause that makes it a bug?

Sure, one of possible problems is described in nspcc-dev/neo-go#3471. The contract is being prohibited from updating (by buggy NeoGo node) because new manifest does not allow the notification that old contract code emits after update. Another unwanted consequence of this bug is: consider the contract being destroyed and calling another contract after that. Without this fix the destroy transaction will be FAULTed. In general, every contract call after update/destroy is affected by this bug.

Also, Jimmy, I'm kindly asking not to modify the PR (this or others). You may leave the comments, and I'm always here to give a response and fix something. But I think we need to come to an agreement before modifying the code (to avoid situations like #3209 (comment)).

[vncoelho]

I understand the change,
However, I see that it would need a HF, otherwise we could fall in the same problem and discussion we recently have had.

[AnnaShaleva]

However, I see that it would need a HF, otherwise we could fall in the same problem and discussion we recently have had.

I understand, but: mainnet/testnet check is in progress (I'll write once it's finished) and this change likely did not affect the mainnet's state since it's hard to meet the trigger criteria. And if no one pushes such transaction to the chain in the nearest future, then it will be safe to update without hardfork. Thus, @roman-khimov's suggestion was to try to avoid introducing hardfork for this change.

[vncoelho]

I understand, but: mainnet/testnet check is in progress (I'll write once it's finished) and this change likely did not affect the mainnet's state since it's hard to meet the trigger criteria. And if no one pushes such transaction to the chain in the nearest future, then it will be safe to update without hardfork. Thus, @roman-khimov's suggestion was to try to avoid introducing hardfork for this change.

Unfortunately, this expectation can not guide our decision, @AnnaShaleva .
Some bad actor can watch github. It is dangerous because we never know when a TX can be pushed. The result can be catastrophic because there could be an attack and outflow from a CEX, for example.
If we had just DeFi in a SOLO-Chain attacks could be "reverted", but that is not the case of current blockchain ecosystem with bridges and etc...

[roman-khimov]

We can have it HF-dependent, no problem with that.

[AnnaShaleva]

Good, then I'll update the code.

[vncoelho]

Ok, that is a first point. Now it is time to discuss the issue itself...aheuaheua

[AnnaShaleva]

We can have it HF-dependent, no problem with that.

Done, ready for review.

[AnnaShaleva]

Suggestions on name are welcomed.

[Jim8y]

I am not familier with any of them, LOL, LGTM

[shargon]

Same as Jimmy

[vang1ong7ang]

i think now it's okay to merge

[vang1ong7ang]

but the necessity of this PR is worth discussing again

i don't think there's any FAULT in the original design. it's just which impl is better

[roman-khimov]

i don't think there's any FAULT in the original design.

It's using wrong manifest for permission checks after the update. And can't work at all after destroy. Contracts can have any code after update/destroy (not supposed to, but can), so either it works correctly or fails in unpredictable manner.

[lingyido]

It's using wrong manifest for permission checks after the update. And can't work at all after destroy. Contracts can have any code after update/destroy (not supposed to, but can), so either it works correctly or fails in unpredictable manner.

Maybe it's by design. And they're actually predictable if they can understand what will happen under current implementation. Running new _deploy code under new Environment and running old update-folllowed code under old Enviroment is somehow reasonable. As you said, one is not supposed to do such an operation. Do we indeed need this new HF for changing this behavior?

What is more curious is how many Environments are there apart from the external-call-permission-control and notification-control. Do they follow the same logic?

[AnnaShaleva]

Do we indeed need this new HF for changing this behavior?

Mainnet is checked and confirmed not to be affected by this bug up to 5484582 height (ref. nspcc-dev/neo-go#3473 (comment)). However, it can be changed at any moment if a suitable transaction will be pushed to mainnet (see #3290 (comment)). And since we decide not to allow state changes anymore, we'd better move it under hardfork.

What is more curious is how many Environments are there apart from the permission-control and notification control.

There's one more explicit call to the executing contract state (see the #3290 (comment)) and a wide set of calls to other executing context-bound things like call flags.

Do they follow the same logic?

Yes, they do, that's why I've created this PR.

[lingyido]

It's better to show an example here for developers to decide which is good.

Let's say. There is an old contract on chain whose code are as follows.

// Permission: allow call Contract_Other's "test"
// Events: Synced

public static void Update(ByteString nefFile, string manifest)
{
    some_code_here(); // this should follow old contract's restrict
    ContractManagement.Update(nefFile, manifest, null);
    some_other_code_here(); // containing notification and external call, what does these code should follow? new contract's or old contract's?
}
public static void destroy() {
    some_code_here(); // this should follow old contract's restrict
    ContractManagement.Destroy();
    some_other_code_here(); // containing notification and external call, what does these code should follow? act like a non-contract? act like the old contract still exists?
}

1. what restriction should some_other_code_here follow for update and destroy?
   a. follow old contract's restriction
   b. follow new contract's restriction for udpate and act like a pure non-contract script for destroy (or fails because contract not exist anymore like what is done now)

2. What if they decided to update the implementation to this way.
   a. follow old contract and succeed for Contract NEW_A, fail for Contract NEW_B
   b. follow new contract and fail for Contract NEW_A, succeed for Contract NEW_B

** Notice that _deploy is automatically called by ContractManagement at the end of ContractManagement.Update's internal implementation. **

Contract NEW_A

// Permission:  NO
// Events:  NO

public static void _deploy(object data, bool update) {
            Synced()
            Contract.Call(Contract_Other, "test", CallFlags.All, new object[] {})
}

Contract NEW_B

// Permission: Contract_Other's "newtest"
// Events:  NewSynced()

public static void _deploy(object data, bool update) {
            NewSynced()
            Contract.Call(Contract_Other, "newtest", CallFlags.All, new object[] {})
}

---

For problem 2, it seems that we should follow 2.b and current implementation for C# follows this way now.

From what I understand, what this PR is trying to change is for problem 1's external call control (the notification control already follows 1.a). The old implementation is 1.b and the new implementation is 1.a. Please correct me if I'm wrong here @AnnaShaleva @roman-khimov .

[roman-khimov]

Execution context must be consistent. Contract is NEF+manifest. If we have a contract loaded into VM it must use appropriate manifest that is was deployed with. We're not changing contract's code (NEF, effectively) on the invocation stack after update, right? We're not aborting its execution after destroy. So, we must not change its manifest. Checking permissions against some new version (or failing if contract is destroyed) when we have old bytecode executing is just wrong.

[lingyido]

After playing with those examples, I stand with @AnnaShaleva if my understanding on that example is right.
It's indeed a serious problem and have the necessity. What's your opinion @vang1ong7ang?

[vang1ong7ang]

okay i'm convinced and let's merge

[lingyido]

There is another external-call method called CallToken, it also reads tokens from old contract state. It mays this change more reasonable.

[vncoelho]

Although simple I stand with @lingyido in his example, I just worry about the change in the logic as in comment #3290 (comment)

Devs should be aware of this change