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 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 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>
neo-project · Jun 4, 2024 · a148dd8 · a148dd8
1 parent fd1edf0
commit a148dd8
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 4 deletions.
diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs
@@ -286,14 +286,16 @@ 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;
+            // Use executing contract state (not the state from native Management), ref. https://github.com/neo-project/neo/pull/3290.
+            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)
+                if (state.Contract?.CanCall(contract, method.Name) == false)
                     throw new InvalidOperationException($"Cannot Call Method {method.Name} Of Contract {contract.Hash} From Contract {CurrentScriptHash}");
             }
 
@@ -306,8 +308,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");

diff --git a/tests/Neo.UnitTests/SmartContract/UT_ApplicationEngine.cs b/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());
+            }
+        }
     }
 }
diff --git a/tests/Neo.UnitTests/SmartContract/UT_InteropService.cs b/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"})
+                            }
                         }
                     }
                 };