From 407c7d5cbf2e33c85f13cd6276c14d48d78a55fd Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 3 Jun 2024 18:58:18 +0300 Subject: [PATCH] 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 https://github.com/nspcc-dev/neo-go/pull/3473. This bug was discovered during the similar problem described in https://github.com/nspcc-dev/neo-go/issues/3471 and fixed in https://github.com/nspcc-dev/neo-go/pull/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 --- src/Neo/Hardfork.cs | 3 +- src/Neo/SmartContract/ApplicationEngine.cs | 10 +- .../SmartContract/UT_ApplicationEngine.cs | 101 ++++++++++++++++++ .../SmartContract/UT_InteropService.cs | 26 ++++- .../SmartContract/UT_Syscalls.cs | 2 +- 5 files changed, 135 insertions(+), 7 deletions(-) diff --git a/src/Neo/Hardfork.cs b/src/Neo/Hardfork.cs index 9ef6a63c1c..7bd3cc0aef 100644 --- a/src/Neo/Hardfork.cs +++ b/src/Neo/Hardfork.cs @@ -15,6 +15,7 @@ public enum Hardfork : byte { HF_Aspidochelone, HF_Basilisk, - HF_Cockatrice + HF_Cockatrice, + HF_Domovoi } } diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index ead04aaba8..2483df8824 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/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(); 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(); 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 index 7639377fe1..0969e12776 100644 --- 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().Contract = new() + { + Manifest = new() + { + Abi = new() { }, + Permissions = new ContractPermission[] + { + new ContractPermission + { + Contract = ContractPermissionDescriptor.Create(scriptHash), + Methods = WildcardContainer.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().Contract = new() + { + Manifest = new() + { + Abi = new() { }, + Permissions = new ContractPermission[] + { + new ContractPermission + { + Contract = ContractPermissionDescriptor.Create(scriptHash), + Methods = WildcardContainer.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 index 0e9916b7d5..51f26d22eb 100644 --- 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.Create(new string[]{"test"}) + } + }; snapshot.AddContract(scriptHash2, contract); } @@ -133,6 +141,14 @@ public void Runtime_GetNotifications_Test() Parameters = System.Array.Empty() } } + }, + Permissions = new ContractPermission[] + { + new ContractPermission + { + Contract = ContractPermissionDescriptor.Create(scriptHash2), + Methods = WildcardContainer.Create(new string[]{"test"}) + } } } }; @@ -202,6 +218,14 @@ public void Runtime_GetNotifications_Test() Parameters = System.Array.Empty() } } + }, + Permissions = new ContractPermission[] + { + new ContractPermission + { + Contract = ContractPermissionDescriptor.Create(scriptHash2), + Methods = WildcardContainer.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); diff --git a/tests/Neo.UnitTests/SmartContract/UT_Syscalls.cs b/tests/Neo.UnitTests/SmartContract/UT_Syscalls.cs index 0a98b5fd66..7791b8acef 100644 --- a/tests/Neo.UnitTests/SmartContract/UT_Syscalls.cs +++ b/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());