From 16bf7c1426bb88b371b08a26abb911fd953556ce Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 2 Jun 2022 09:00:59 +0300 Subject: [PATCH] core: adjust contract script check on deploy Reference implementation doesn't panic if the method offset is out of the contract script bounds, see: https://github.com/neo-project/neo/blob/736c346b9d8b1404f10023eeecb3a8e92ae0c542/src/neo/SmartContract/Helper.cs#L82 and https://github.com/neo-project/neo-vm/blob/a65487fa56be3eccb2c1dbfec5dcdd71b8a05fde/src/Neo.VM/Script.cs#L146. This commit fixes T5 statediff at block #125000. Neo-go node FAULTed the deploying transaction: ``` { "version" : 0, "sysfee" : "1000106065", "validuntilblock" : 130758, "script" : "DdMDeyJuYW1lIjoiTmVwMTdUb2tlbiIsImdyb3VwcyI6W10sImZlYXR1cmVzIjp7fSwic3VwcG9ydGVkc3RhbmRhcmRzIjpbIk5FUC0xNyJdLCJhYmkiOnsibWV0aG9kcyI6W3sibmFtZSI6InN5bWJvbCIsInBhcmFtZXRlcnMiOltdLCJyZXR1cm50eXBlIjoiU3RyaW5nIiwib2Zmc2V0IjoyMiwic2FmZSI6dHJ1ZX0seyJuYW1lIjoiZGVjaW1hbHMiLCJwYXJhbWV0ZXJzIjpbXSwicmV0dXJudHlwZSI6IkludGVnZXIiLCJvZmZzZXQiOjIyLCJzYWZlIjp0cnVlfSx7Im5hbWUiOiJ0b3RhbFN1cHBseSIsInBhcmFtZXRlcnMiOltdLCJyZXR1cm50eXBlIjoiSW50ZWdlciIsIm9mZnNldCI6MjIsInNhZmUiOnRydWV9LHsibmFtZSI6ImJhbGFuY2VPZiIsInBhcmFtZXRlcnMiOlt7Im5hbWUiOiJvd25lciIsInR5cGUiOiJIYXNoMTYwIn1dLCJyZXR1cm50eXBlIjoiSW50ZWdlciIsIm9mZnNldCI6MjIsInNhZmUiOnRydWV9LHsibmFtZSI6InRyYW5zZmVyIiwicGFyYW1ldGVycyI6W3sibmFtZSI6ImZyb20iLCJ0eXBlIjoiSGFzaDE2MCJ9LHsibmFtZSI6InRvIiwidHlwZSI6Ikhhc2gxNjAifSx7Im5hbWUiOiJhbW91bnQiLCJ0eXBlIjoiSW50ZWdlciJ9LHsibmFtZSI6ImRhdGEiLCJ0eXBlIjoiQW55In1dLCJyZXR1cm50eXBlIjoiQm9vbGVhbiIsIm9mZnNldCI6MjIsInNhZmUiOmZhbHNlfV0sImV2ZW50cyI6W3sibmFtZSI6IlRyYW5zZmVyIiwicGFyYW1ldGVycyI6W3sibmFtZSI6ImZyb20iLCJ0eXBlIjoiSGFzaDE2MCJ9LHsibmFtZSI6InRvIiwidHlwZSI6Ikhhc2gxNjAifSx7Im5hbWUiOiJhbW91bnQiLCJ0eXBlIjoiSW50ZWdlciJ9XX1dfSwicGVybWlzc2lvbnMiOlt7ImNvbnRyYWN0IjoiKiIsIm1ldGhvZHMiOiIqIn1dLCJ0cnVzdHMiOltdLCJleHRyYSI6eyJlbWFpbCI6ImRldmVsb3BlckBuZW8ub3JnIiwiYXV0aG9yIjoibGF6eW5vZGUiLCJkZXNjcmlwdGlvbiI6IkEgU2ltcGxlIE5lcC0xNyBDb250cmFjdCJ9fQyyTkVGM25lb21sAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABOaHR0cHM6Ly9naXRodWIuY29tL2xhenlub2RlL25lb21sL2Jsb2IvZGV2L2V4YW1wbGVzL2EuZnVuY3Rpb24uc2ltcGxlbmVwMTcueG1sAAAAABYMBU5lb01MQBhAAgDh9QVAATkFQBFAkA456BLAHwwGZGVwbG95DBT9o/pDRupTKiWPxJfdrdtkN8n9/0FifVtS", "hash" : "0x40302bcf2021f63a1c24f6009e154c3200f73ad2fe1462d7d599145823dbfa7e", "witnesses" : [ { "verification" : "DCECExn08eznGBdguHbcwI+R2//EtVdDx4qf6CeizHqOJgBBVuezJw==", "invocation" : "DEBtoq+T9NrammQjuYnifco7KHCTk2v+woEJqJCUMr9IscS7PaZaN3FNzSt11yUglIi3T0CJ17KwArBOBvJ8kwq2" } ], "attributes" : [], "signers" : [ { "scopes" : "None", "account" : "0x13a192c56738900f9918d7f1ec07d9d8c278b804" } ], "size" : 1360, "nonce" : 1829882407, "sender" : "NLLvsqs7AyBNmQT6NThUxYWDFwV5b1evaK", "netfee" : "234352" } ``` Transaction script contains malformed contract manifest (all methods offsets are set to be 22, while the contract script lenght is 22): ``` { "name" : "Nep17Token", "groups" : [], "extra" : { "description" : "A Simple Nep-17 Contract", "email" : "developer@neo.org", "author" : "lazynode" }, "permissions" : [ { "contract" : "*", "methods" : "*" } ], "features" : {}, "supportedstandards" : [ "NEP-17" ], "abi" : { "events" : [ { "parameters" : [ { "name" : "from", "type" : "Hash160" }, { "type" : "Hash160", "name" : "to" }, { "name" : "amount", "type" : "Integer" } ], "name" : "Transfer" } ], "methods" : [ { "safe" : true, "offset" : 22, "name" : "symbol", "returntype" : "String", "parameters" : [] }, { "returntype" : "Integer", "parameters" : [], "safe" : true, "offset" : 22, "name" : "decimals" }, { "parameters" : [], "returntype" : "Integer", "name" : "totalSupply", "safe" : true, "offset" : 22 }, { "parameters" : [ { "name" : "owner", "type" : "Hash160" } ], "returntype" : "Integer", "name" : "balanceOf", "offset" : 22, "safe" : true }, { "name" : "transfer", "offset" : 22, "safe" : false, "parameters" : [ { "type" : "Hash160", "name" : "from" }, { "name" : "to", "type" : "Hash160" }, { "name" : "amount", "type" : "Integer" }, { "name" : "data", "type" : "Any" } ], "returntype" : "Boolean" } ] }, "trusts" : [] } ``` --- pkg/core/native/management.go | 2 +- pkg/core/native/native_test/management_test.go | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/core/native/management.go b/pkg/core/native/management.go index b30266624..eee25bef6 100644 --- a/pkg/core/native/management.go +++ b/pkg/core/native/management.go @@ -614,7 +614,7 @@ func checkScriptAndMethods(script []byte, methods []manifest.Method) error { offsets := bitfield.New(l) for i := range methods { if methods[i].Offset >= l { - return errors.New("out of bounds method offset") + continue } offsets.Set(methods[i].Offset) } diff --git a/pkg/core/native/native_test/management_test.go b/pkg/core/native/native_test/management_test.go index e2c690c16..2f212c9d6 100644 --- a/pkg/core/native/native_test/management_test.go +++ b/pkg/core/native/native_test/management_test.go @@ -135,13 +135,20 @@ func TestManagement_ContractDeploy(t *testing.T) { badManifest := cs1.Manifest badManifest.ABI.Methods = make([]manifest.Method, len(cs1.Manifest.ABI.Methods)) copy(badManifest.ABI.Methods, cs1.Manifest.ABI.Methods) - badManifest.ABI.Methods[0].Offset = 100500 // out of bounds + badManifest.ABI.Methods[0].Offset = 100500 // out of bounds, but it's OK, this method will not be checked then. manifB, err := json.Marshal(&badManifest) require.NoError(t, err) - managementInvoker.InvokeFail(t, "out of bounds method offset", "deploy", nefBytes, manifB) - }) + tx := c.PrepareInvokeNoSign(t, "deploy", nefBytes, manifB) + tx.Signers = []transaction.Signer{{}} // Need dummy signer to deploy. + b := c.NewUnsignedBlock(t, tx) + ic := c.Chain.GetTestVM(trigger.Application, tx, b) + t.Cleanup(ic.Finalize) + ic.VM.LoadWithFlags(tx.Script, callflag.All) + err = ic.VM.Run() + require.NoError(t, err) + }) t.Run("bad methods in manifest 2", func(t *testing.T) { var badManifest = cs1.Manifest badManifest.ABI.Methods = make([]manifest.Method, len(cs1.Manifest.ABI.Methods))