SmartContract: use executing contract state to check permissions

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 is moved under D hardfork to avoid state diff issues on
nodes update. Although it should be noted that it's hard to meet the
trigger criteria.

A port of nspcc-dev/neo-go#3473. This bug was
discovered during the similar problem described in
nspcc-dev/neo-go#3471 and fixed in
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).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

src/Neo/Hardfork.cs

@@ -15,6 +15,7 @@ public enum Hardfork : byte
     {
         HF_Aspidochelone,
         HF_Basilisk,
-        HF_Cockatrice
+        HF_Cockatrice,
+        HF_Domovoi
     }
 }

src/Neo/SmartContract/ApplicationEngine.cs

@@ -286,14 +286,18 @@ private ExecutionContext CallContractInternal(ContractState contract, ContractMe
             if (NativeContract.Policy.IsBlocked(Snapshot, contract.Hash))
                 throw new InvalidOperationException($"The contract {contract.Hash} has been blocked.");
 
+            ExecutionContext currentContext = CurrentContext;
+            ExecutionContextState state = currentContext.GetState<ExecutionContextState>();
             if (method.Safe)
             {
                 flags &= ~(CallFlags.WriteStates | CallFlags.AllowNotify);
             }
             else
             {
-                ContractState currentContract = NativeContract.ContractManagement.GetContract(Snapshot, CurrentScriptHash);
-                if (currentContract?.CanCall(contract, method.Name) == false)
+                var executingContract = IsHardforkEnabled(Hardfork.HF_Domovoi)
+                ? state.Contract // use executing contract state to avoid possible contract update/destroy side-effects, ref. https://github.com/neo-project/neo/pull/3290.
+                : NativeContract.ContractManagement.GetContract(Snapshot, CurrentScriptHash);
+                if (executingContract?.CanCall(contract, method.Name) == false)
                     throw new InvalidOperationException($"Cannot Call Method {method.Name} Of Contract {contract.Hash} From Contract {CurrentScriptHash}");
             }
 
@@ -306,8 +310,6 @@ private ExecutionContext CallContractInternal(ContractState contract, ContractMe
                 invocationCounter[contract.Hash] = 1;
             }
 
-            ExecutionContext currentContext = CurrentContext;
-            ExecutionContextState state = currentContext.GetState<ExecutionContextState>();
             CallFlags callingFlags = state.CallFlags;
 
             if (args.Count != method.Parameters.Length) throw new InvalidOperationException($"Method {method} Expects {method.Parameters.Length} Arguments But Receives {args.Count} Arguments");

tests/Neo.UnitTests/SmartContract/UT_ApplicationEngine.cs

@@ -12,6 +12,9 @@
 using FluentAssertions;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Neo.SmartContract;
+using Neo.SmartContract.Manifest;
+using Neo.UnitTests.Extensions;
+using Neo.VM;
 using System;
 using System.Collections.Immutable;
 using System.Linq;
@@ -103,5 +106,103 @@ public void TestCheckingHardfork()
                 (setting[sortedHardforks[i]] > setting[sortedHardforks[i + 1]]).Should().Be(false);
             }
         }
+
+        [TestMethod]
+        public void TestSystem_Contract_Call_Permissions()
+        {
+            UInt160 scriptHash;
+            var snapshot = TestBlockchain.GetTestSnapshot();
+
+            // Setup: put a simple contract to the storage.
+            using (var script = new ScriptBuilder())
+            {
+                // Push True on stack and return.
+                script.EmitPush(true);
+                script.Emit(OpCode.RET);
+
+                // Mock contract and put it to the Managemant's storage.
+                scriptHash = script.ToArray().ToScriptHash();
+
+                snapshot.DeleteContract(scriptHash);
+                var contract = TestUtils.GetContract(script.ToArray(), TestUtils.CreateManifest("test", ContractParameterType.Any));
+                contract.Manifest.Abi.Methods = new[]
+                {
+                    new ContractMethodDescriptor
+                    {
+                        Name = "disallowed",
+                        Parameters = new ContractParameterDefinition[]{}
+                    },
+                    new ContractMethodDescriptor
+                    {
+                        Name = "test",
+                        Parameters = new ContractParameterDefinition[]{}
+                    }
+                };
+                snapshot.AddContract(scriptHash, contract);
+            }
+
+            // Disallowed method call.
+            using (var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, null, ProtocolSettings.Default))
+            using (var script = new ScriptBuilder())
+            {
+                // Build call script calling disallowed method.
+                script.EmitDynamicCall(scriptHash, "disallowed");
+
+                // Mock executing state to be a contract-based.
+                engine.LoadScript(script.ToArray());
+                engine.CurrentContext.GetState<ExecutionContextState>().Contract = new()
+                {
+                    Manifest = new()
+                    {
+                        Abi = new() { },
+                        Permissions = new ContractPermission[]
+                        {
+                            new ContractPermission
+                            {
+                                Contract = ContractPermissionDescriptor.Create(scriptHash),
+                                Methods = WildcardContainer<string>.Create(new string[]{"test"}) // allowed to call only "test" method of the target contract.
+                            }
+                        }
+                    }
+                };
+                var currentScriptHash = engine.EntryScriptHash;
+
+                Assert.AreEqual(VMState.FAULT, engine.Execute());
+                Assert.IsTrue(engine.FaultException.ToString().Contains($"Cannot Call Method disallowed Of Contract {scriptHash.ToString()}"));
+            }
+
+            // Allowed method call.
+            using (var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, null, ProtocolSettings.Default))
+            using (var script = new ScriptBuilder())
+            {
+                // Build call script.
+                script.EmitDynamicCall(scriptHash, "test");
+
+                // Mock executing state to be a contract-based.
+                engine.LoadScript(script.ToArray());
+                engine.CurrentContext.GetState<ExecutionContextState>().Contract = new()
+                {
+                    Manifest = new()
+                    {
+                        Abi = new() { },
+                        Permissions = new ContractPermission[]
+                        {
+                            new ContractPermission
+                            {
+                                Contract = ContractPermissionDescriptor.Create(scriptHash),
+                                Methods = WildcardContainer<string>.Create(new string[]{"test"}) // allowed to call only "test" method of the target contract.
+                            }
+                        }
+                    }
+                };
+                var currentScriptHash = engine.EntryScriptHash;
+
+                Assert.AreEqual(VMState.HALT, engine.Execute());
+                Assert.AreEqual(1, engine.ResultStack.Count);
+                Assert.IsInstanceOfType(engine.ResultStack.Peek(), typeof(VM.Types.Boolean));
+                var res = (VM.Types.Boolean)engine.ResultStack.Pop();
+                Assert.IsTrue(res.GetBoolean());
+            }
+        }
     }
 }

tests/Neo.UnitTests/SmartContract/UT_InteropService.cs

@@ -71,6 +71,14 @@ public void Runtime_GetNotifications_Test()
                         }
                     }
                 };
+                contract.Manifest.Permissions = new ContractPermission[]
+                {
+                    new ContractPermission
+                    {
+                        Contract = ContractPermissionDescriptor.Create(scriptHash2),
+                        Methods = WildcardContainer<string>.Create(new string[]{"test"})
+                    }
+                };
                 snapshot.AddContract(scriptHash2, contract);
             }
 
@@ -133,6 +141,14 @@ public void Runtime_GetNotifications_Test()
                                     Parameters = System.Array.Empty<ContractParameterDefinition>()
                                 }
                             }
+                        },
+                        Permissions = new ContractPermission[]
+                        {
+                            new ContractPermission
+                            {
+                                Contract = ContractPermissionDescriptor.Create(scriptHash2),
+                                Methods = WildcardContainer<string>.Create(new string[]{"test"})
+                            }
                         }
                     }
                 };
@@ -202,6 +218,14 @@ public void Runtime_GetNotifications_Test()
                                     Parameters = System.Array.Empty<ContractParameterDefinition>()
                                 }
                             }
+                        },
+                        Permissions = new ContractPermission[]
+                        {
+                            new ContractPermission
+                            {
+                                Contract = ContractPermissionDescriptor.Create(scriptHash2),
+                                Methods = WildcardContainer<string>.Create(new string[]{"test"})
+                            }
                         }
                     }
                 };
@@ -642,7 +666,7 @@ public void TestContract_Call()
             var args = new VM.Types.Array { 0, 1 };
             var state = TestUtils.GetContract(method, args.Count);
 
-            var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot);
+            var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, null, ProtocolSettings.Default);
             engine.LoadScript(new byte[] { 0x01 });
             engine.Snapshot.AddContract(state.Hash, state);
 

tests/Neo.UnitTests/SmartContract/UT_Syscalls.cs

@@ -257,7 +257,7 @@ public void System_Runtime_GetInvocationCounter()
 
                 // Execute
 
-                var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot);
+                var engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, null, ProtocolSettings.Default);
                 engine.LoadScript(script.ToArray());
                 Assert.AreEqual(VMState.HALT, engine.Execute());