From dc4d5a2dc6ed387cd9266083568a501c7d77bce9 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Sat, 1 Jun 2024 13:07:03 +0300 Subject: [PATCH] interop: use currently executing contract state for permissions check It's not correct to use an updated contract state got from 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 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, but I'd put it under the hardfork anyway. Signed-off-by: Anna Shaleva --- pkg/core/interop/contract/call.go | 15 +++- pkg/core/interop/contract/call_test.go | 112 +++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/pkg/core/interop/contract/call.go b/pkg/core/interop/contract/call.go index 61713c533..60f8ac9c5 100644 --- a/pkg/core/interop/contract/call.go +++ b/pkg/core/interop/contract/call.go @@ -6,6 +6,7 @@ import ( "math/big" "strings" + "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core/dao" "github.com/nspcc-dev/neo-go/pkg/core/interop" "github.com/nspcc-dev/neo-go/pkg/core/native/nativenames" @@ -77,12 +78,18 @@ func callInternal(ic *interop.Context, cs *state.Contract, name string, f callfl if md.Safe { f &^= (callflag.WriteStates | callflag.AllowNotify) } else if ctx := ic.VM.Context(); ctx != nil && ctx.IsDeployed() { - curr, err := ic.GetContract(ic.VM.GetCurrentScriptHash()) - if err == nil { - if !curr.Manifest.CanCall(cs.Hash, &cs.Manifest, name) { - return errors.New("disallowed method call") + var mfst *manifest.Manifest + if ic.IsHardforkEnabled(config.HFDomovoi) { + mfst = ctx.GetManifest() + } else { + curr, err := ic.GetContract(ic.VM.GetCurrentScriptHash()) + if err == nil { + mfst = &curr.Manifest } } + if mfst != nil && !mfst.CanCall(cs.Hash, &cs.Manifest, name) { + return errors.New("disallowed method call") + } } return callExFromNative(ic, ic.VM.GetCurrentScriptHash(), cs, name, args, f, hasReturn, isDynamic, false) } diff --git a/pkg/core/interop/contract/call_test.go b/pkg/core/interop/contract/call_test.go index 451d5b41d..c1f6034f3 100644 --- a/pkg/core/interop/contract/call_test.go +++ b/pkg/core/interop/contract/call_test.go @@ -11,6 +11,7 @@ import ( "github.com/nspcc-dev/neo-go/internal/contracts" "github.com/nspcc-dev/neo-go/internal/random" "github.com/nspcc-dev/neo-go/pkg/compiler" + "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/core/interop" "github.com/nspcc-dev/neo-go/pkg/core/interop/contract" @@ -173,6 +174,117 @@ func TestCall(t *testing.T) { }) } +func TestSystemContractCall_Permissions(t *testing.T) { + check := func(t *testing.T, cfg func(*config.Blockchain), shouldUpdateFail bool) { + bc, acc := chain.NewSingleWithCustomConfig(t, cfg) + e := neotest.NewExecutor(t, bc, acc, acc) + + // Contract A has an unsafe method. + srcA := `package contractA + func RetOne() int { + return 1 + }` + ctrA := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(srcA), &compiler.Options{ + NoEventsCheck: true, + NoPermissionsCheck: true, + Name: "contractA", + }) + e.DeployContract(t, ctrA, nil) + + var hashAStr string + for i := 0; i < util.Uint160Size; i++ { + hashAStr += fmt.Sprintf("%#x", ctrA.Hash[i]) + if i != util.Uint160Size-1 { + hashAStr += ", " + } + } + + // Contract B has a method that calls contract's A and another update method that + // calls contract's A after B's update. + srcB := `package contractB + import ( + "github.com/nspcc-dev/neo-go/pkg/interop" + "github.com/nspcc-dev/neo-go/pkg/interop/contract" + "github.com/nspcc-dev/neo-go/pkg/interop/native/management" + ) + func CallRetOne() int { + res := contract.Call(interop.Hash160{` + hashAStr + `}, "retOne", contract.All).(int) + return res + } + func Update(nef []byte, manifest []byte) int { + management.Update(nef, manifest) + res := contract.Call(interop.Hash160{` + hashAStr + `}, "retOne", contract.All).(int) + return res + }` + ctrB := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(srcB), &compiler.Options{ + Name: "contractB", + NoEventsCheck: true, + NoPermissionsCheck: true, + Permissions: []manifest.Permission{ + { + Contract: manifest.PermissionDesc{Type: manifest.PermissionHash, Value: ctrA.Hash}, + Methods: manifest.WildStrings{Value: []string{"retOne"}}, + }, + { + Methods: manifest.WildStrings{Value: []string{"update"}}, + }, + }, + }) + e.DeployContract(t, ctrB, nil) + ctrBInvoker := e.ValidatorInvoker(ctrB.Hash) + + // ctrBUpdated differs from ctrB in that it has no permission to call retOne method of ctrA + ctrBUpdated := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(srcB), &compiler.Options{ + Name: "contractB", + NoEventsCheck: true, + NoPermissionsCheck: true, + Permissions: []manifest.Permission{ + { + Contract: manifest.PermissionDesc{Type: manifest.PermissionHash, Value: ctrA.Hash}, + Methods: manifest.WildStrings{Value: []string{}}, + }, + { + Methods: manifest.WildStrings{Value: []string{"update"}}, + }, + }, + }) + + // Call to A before B update should HALT. + ctrBInvoker.Invoke(t, stackitem.Make(1), "callRetOne") + + // Call to A in the same context as B update should HALT. + n, err := ctrBUpdated.NEF.Bytes() + require.NoError(t, err) + m, err := json.Marshal(ctrBUpdated.Manifest) + require.NoError(t, err) + if shouldUpdateFail { + ctrBInvoker.InvokeFail(t, "System.Contract.Call failed: disallowed method call", "update", n, m) + } else { + ctrBInvoker.Invoke(t, stackitem.Make(1), "update", n, m) + } + + // If contract is updated, then all to A after B update should FAULT (manifest + // is updated, no permission to call retOne method). + if !shouldUpdateFail { + ctrBInvoker.InvokeFail(t, "System.Contract.Call failed: disallowed method call", "callRetOne") + } + } + + // Pre-Domovoi behaviour: an updated contract state is used for permissions check. + check(t, func(cfg *config.Blockchain) { + cfg.Hardforks = map[string]uint32{ + config.HFDomovoi.String(): 100500, + } + }, true) + + // Post-Domovoi behaviour: an executing contract state is used for permissions check. + check(t, func(cfg *config.Blockchain) { + cfg.Hardforks = map[string]uint32{ + config.HFDomovoi.String(): 0, + } + }, false) +} + func TestLoadToken(t *testing.T) { bc, acc := chain.NewSingle(t) e := neotest.NewExecutor(t, bc, acc, acc)