Merge pull request #2720 from nspcc-dev/notifications-check

compiler: enforce runtime.Notify parameters cast to proper type

docs/compiler.md

@@ -293,7 +293,11 @@ By default, compiler performs some sanity checks. Most of the time
 it will report missing events and/or parameter type mismatch.
 It isn't prohibited to use a variable as an event name in code, but it will prevent
 the compiler from analyzing the event. It is better to use either constant or string literal.
-The check can be disabled with `--no-events` flag.
+It isn't prohibited to use ellipsis expression as an event arguments, but it will also
+prevent the compiler from analyzing the event. It is better to provide arguments directly
+without `...`. The type conversion code will be emitted for checked events, it will cast
+argument types to ones specified in the contract manifest. These checks and conversion can
+be disabled with `--no-events` flag.
 
 ##### Permissions
 Each permission specifies contracts and methods allowed for this permission.

pkg/compiler/compiler.go

@@ -13,6 +13,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/nspcc-dev/neo-go/pkg/smartcontract"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/binding"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest/standard"
@@ -363,6 +364,9 @@ func CreateManifest(di *DebugInfo, o *Options) (*manifest.Manifest, error) {
 						name, len(ev.Parameters), len(argsList[i]))
 				}
 				for j := range ev.Parameters {
+					if ev.Parameters[j].Type == smartcontract.AnyType {
+						continue
+					}
 					expected := ev.Parameters[j].Type.String()
 					if argsList[i][j] != expected {
 						return nil, fmt.Errorf("event '%s' should have '%s' as type of %d parameter, "+

pkg/compiler/compiler_test.go

@@ -174,6 +174,16 @@ func TestEventWarnings(t *testing.T) {
 		})
 		require.Error(t, err)
 	})
+	t.Run("any parameter type", func(t *testing.T) {
+		_, err = compiler.CreateManifest(di, &compiler.Options{
+			ContractEvents: []manifest.Event{{
+				Name:       "Event",
+				Parameters: []manifest.Parameter{manifest.NewParameter("number", smartcontract.AnyType)},
+			}},
+			Name: "payable",
+		})
+		require.NoError(t, err)
+	})
 	t.Run("good", func(t *testing.T) {
 		_, err = compiler.CreateManifest(di, &compiler.Options{
 			ContractEvents: []manifest.Event{{
@@ -220,6 +230,25 @@ func TestEventWarnings(t *testing.T) {
 			require.NoError(t, err)
 		})
 	})
+	t.Run("variadic event args via ellipsis", func(t *testing.T) {
+		src := `package payable
+		import "github.com/nspcc-dev/neo-go/pkg/interop/runtime"
+		func Main() {
+			runtime.Notify("Event", []interface{}{1}...)
+		}`
+
+		_, di, err := compiler.CompileWithOptions("eventTest.go", strings.NewReader(src), nil)
+		require.NoError(t, err)
+
+		_, err = compiler.CreateManifest(di, &compiler.Options{
+			Name: "eventTest",
+			ContractEvents: []manifest.Event{{
+				Name:       "Event",
+				Parameters: []manifest.Parameter{manifest.NewParameter("number", smartcontract.IntegerType)},
+			}},
+		})
+		require.NoError(t, err)
+	})
 }
 
 func TestNotifyInVerify(t *testing.T) {

pkg/compiler/inline.go

@@ -11,6 +11,7 @@ import (
 	"github.com/nspcc-dev/neo-go/pkg/util"
 	"github.com/nspcc-dev/neo-go/pkg/vm/emit"
 	"github.com/nspcc-dev/neo-go/pkg/vm/opcode"
+	"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
 )
 
 // inlineCall inlines call of n for function represented by f.
@@ -36,7 +37,8 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) {
 	pkg := c.packageCache[f.pkg.Path()]
 	sig := c.typeOf(n.Fun).(*types.Signature)
 
-	c.processStdlibCall(f, n.Args)
+	hasVarArgs := !n.Ellipsis.IsValid()
+	eventParams := c.processStdlibCall(f, n.Args, !hasVarArgs)
 
 	// When inlined call is used during global initialization
 	// there is no func scope, thus this if.
@@ -68,7 +70,6 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) {
 			scope:     oldScope,
 		})
 	}
-	hasVarArgs := !n.Ellipsis.IsValid()
 	needPack := sig.Variadic() && hasVarArgs
 	for i := range n.Args {
 		c.scope.vars.locals = oldScope
@@ -102,6 +103,11 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) {
 		c.scope.vars.locals = oldScope
 		for i := sig.Params().Len() - 1; i < len(n.Args); i++ {
 			ast.Walk(c, n.Args[i])
+			// In case of runtime.Notify, its arguments need to be converted to proper type.
+			// i's initial value is 1 (variadic args start).
+			if eventParams != nil && eventParams[i-1] != nil {
+				c.emitConvert(*eventParams[i-1])
+			}
 		}
 		c.scope.vars.locals = newScope
 		c.packVarArgs(n, sig)
@@ -129,51 +135,91 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) {
 	c.pkgInfoInline = c.pkgInfoInline[:len(c.pkgInfoInline)-1]
 }
 
-func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr) {
+func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr, hasEllipsis bool) []*stackitem.Type {
 	if f == nil {
-		return
+		return nil
 	}
 
+	var eventParams []*stackitem.Type
 	if f.pkg.Path() == interopPrefix+"/runtime" && (f.name == "Notify" || f.name == "Log") {
-		c.processNotify(f, args)
+		eventParams = c.processNotify(f, args, hasEllipsis)
 	}
 
 	if f.pkg.Path() == interopPrefix+"/contract" && f.name == "Call" {
 		c.processContractCall(f, args)
 	}
+	return eventParams
 }
 
-func (c *codegen) processNotify(f *funcScope, args []ast.Expr) {
+// processNotify checks whether notification emitting rules are met and returns expected
+// notification signature if found.
+func (c *codegen) processNotify(f *funcScope, args []ast.Expr, hasEllipsis bool) []*stackitem.Type {
 	if c.scope != nil && c.isVerifyFunc(c.scope.decl) &&
 		c.scope.pkg == c.mainPkg.Types && (c.buildInfo.options == nil || !c.buildInfo.options.NoEventsCheck) {
 		c.prog.Err = fmt.Errorf("runtime.%s is not allowed in `Verify`", f.name)
-		return
+		return nil
 	}
 
 	if f.name == "Log" {
-		return
+		return nil
 	}
 
-	// Sometimes event name is stored in a var.
+	// Sometimes event name is stored in a var. Or sometimes event args are provided
-	// Skip in this case.
+	// via ellipses (`slice...`).
+	// Skip in this case.  Also, don't enforce runtime.Notify parameters conversion.
 	tv := c.typeAndValueOf(args[0])
-	if tv.Value == nil {
+	if tv.Value == nil || hasEllipsis {
-		return
+		return nil
 	}
 
 	params := make([]string, 0, len(args[1:]))
+	vParams := make([]*stackitem.Type, 0, len(args[1:]))
 	for _, p := range args[1:] {
-		st, _, _ := c.scAndVMTypeFromExpr(p)
+		st, vt, _ := c.scAndVMTypeFromExpr(p)
 		params = append(params, st.String())
+		vParams = append(vParams, &vt)
 	}
 
 	name := constant.StringVal(tv.Value)
 	if len(name) > runtime.MaxEventNameLen {
 		c.prog.Err = fmt.Errorf("event name '%s' should be less than %d",
 			name, runtime.MaxEventNameLen)
-		return
+		return nil
+	}
+	var eventFound bool
+	if c.buildInfo.options != nil && c.buildInfo.options.ContractEvents != nil && !c.buildInfo.options.NoEventsCheck {
+		for _, e := range c.buildInfo.options.ContractEvents {
+			if e.Name == name && len(e.Parameters) == len(vParams) {
+				eventFound = true
+				for i, scParam := range e.Parameters {
+					expectedType := scParam.Type.ConvertToStackitemType()
+					// No need to cast if the desired type is unknown.
+					if expectedType == stackitem.AnyT ||
+						// Do not cast if desired type is Interop, the actual type is likely to be Any, leave the resolving to runtime.Notify.
+						expectedType == stackitem.InteropT ||
+						// No need to cast if actual parameter type matches the desired one.
+						*vParams[i] == expectedType ||
+						// expectedType doesn't contain Buffer anyway, but if actual variable type is Buffer,
+						// then runtime.Notify will convert it to ByteArray automatically, thus no need to emit conversion code.
+						(*vParams[i] == stackitem.BufferT && expectedType == stackitem.ByteArrayT) {
+						vParams[i] = nil
+					} else {
+						// For other cases the conversion code will be emitted using vParams...
+						vParams[i] = &expectedType
+						// ...thus, update emitted notification info in advance.
+						params[i] = scParam.Type.String()
+					}
+				}
+			}
+		}
 	}
 	c.emittedEvents[name] = append(c.emittedEvents[name], params)
+	// Do not enforce perfect expected/actual events match on this step, the final
+	// check wil be performed after compilation if --no-events option is off.
+	if eventFound {
+		return vParams
+	}
+	return nil
 }
 
 func (c *codegen) processContractCall(f *funcScope, args []ast.Expr) {

pkg/compiler/interop_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"math/big"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -22,6 +23,7 @@ import (
 	cinterop "github.com/nspcc-dev/neo-go/pkg/interop"
 	"github.com/nspcc-dev/neo-go/pkg/neotest"
 	"github.com/nspcc-dev/neo-go/pkg/neotest/chain"
+	"github.com/nspcc-dev/neo-go/pkg/smartcontract"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/callflag"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger"
@@ -548,3 +550,145 @@ func TestCallWithVersion(t *testing.T) {
 		c.InvokeFail(t, "contract version mismatch", "callWithVersion", policyH.BytesBE(), 1, "getExecFeeFactor")
 	})
 }
+
+func TestForcedNotifyArgumentsConversion(t *testing.T) {
+	const methodWithEllipsis = "withEllipsis"
+	const methodWithoutEllipsis = "withoutEllipsis"
+	check := func(t *testing.T, method string, targetSCParamTypes []smartcontract.ParamType, expectedVMParamTypes []stackitem.Type, noEventsCheck bool) {
+		bc, acc := chain.NewSingle(t)
+		e := neotest.NewExecutor(t, bc, acc, acc)
+		src := `package foo
+		import "github.com/nspcc-dev/neo-go/pkg/interop/runtime"
+		const arg4 = 4			// Const value.
+		func WithoutEllipsis() {
+			var arg0 int		// Default value.
+			var arg1 int = 1	// Initialized value.
+			arg2 := 2			// Short decl.
+			var arg3 int
+			arg3 = 3			// Declare first, change value afterwards.
+			runtime.Notify("withoutEllipsis", arg0, arg1, arg2, arg3, arg4, 5, f(6))	// The fifth argument is basic literal.
+		}
+		func WithEllipsis() {
+			arg := []interface{}{0, 1, f(2), 3, 4, 5, 6}
+			runtime.Notify("withEllipsis", arg...)
+		}
+		func f(i int) int {
+			return i
+		}`
+		count := len(targetSCParamTypes)
+		if count != len(expectedVMParamTypes) {
+			t.Fatalf("parameters count mismatch: %d vs %d", count, len(expectedVMParamTypes))
+		}
+		scParams := make([]manifest.Parameter, len(targetSCParamTypes))
+		vmParams := make([]stackitem.Item, len(expectedVMParamTypes))
+		for i := range scParams {
+			scParams[i] = manifest.Parameter{
+				Name: strconv.Itoa(i),
+				Type: targetSCParamTypes[i],
+			}
+			defaultValue := stackitem.NewBigInteger(big.NewInt(int64(i)))
+			var (
+				val stackitem.Item
+				err error
+			)
+			if expectedVMParamTypes[i] == stackitem.IntegerT {
+				val = defaultValue
+			} else {
+				val, err = defaultValue.Convert(expectedVMParamTypes[i]) // exactly the same conversion should be emitted by compiler and performed by the contract code.
+				require.NoError(t, err)
+			}
+			vmParams[i] = val
+		}
+		ctr := neotest.CompileSource(t, e.CommitteeHash, strings.NewReader(src), &compiler.Options{
+			Name: "Helper",
+			ContractEvents: []manifest.Event{
+				{
+					Name:       methodWithoutEllipsis,
+					Parameters: scParams,
+				},
+				{
+					Name:       methodWithEllipsis,
+					Parameters: scParams,
+				},
+			},
+			NoEventsCheck: noEventsCheck,
+		})
+		e.DeployContract(t, ctr, nil)
+		c := e.CommitteeInvoker(ctr.Hash)
+
+		t.Run(method, func(t *testing.T) {
+			h := c.Invoke(t, stackitem.Null{}, method)
+			aer := c.GetTxExecResult(t, h)
+			require.Equal(t, 1, len(aer.Events))
+			require.Equal(t, stackitem.NewArray(vmParams), aer.Events[0].Item)
+		})
+	}
+	checkSingleType := func(t *testing.T, method string, targetSCEventType smartcontract.ParamType, expectedVMType stackitem.Type, noEventsCheck ...bool) {
+		count := 7
+		scParams := make([]smartcontract.ParamType, count)
+		vmParams := make([]stackitem.Type, count)
+		for i := range scParams {
+			scParams[i] = targetSCEventType
+			vmParams[i] = expectedVMType
+		}
+		var noEvents bool
+		if len(noEventsCheck) > 0 {
+			noEvents = noEventsCheck[0]
+		}
+		check(t, method, scParams, vmParams, noEvents)
+	}
+
+	t.Run("good, single type, default values", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.IntegerType, stackitem.IntegerT)
+	})
+	t.Run("good, single type, conversion to BooleanT", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.BoolType, stackitem.BooleanT)
+	})
+	t.Run("good, single type, Hash160Type->ByteArray", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.Hash160Type, stackitem.ByteArrayT)
+	})
+	t.Run("good, single type, Hash256Type->ByteArray", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.Hash256Type, stackitem.ByteArrayT)
+	})
+	t.Run("good, single type, Signature->ByteArray", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.SignatureType, stackitem.ByteArrayT)
+	})
+	t.Run("good, single type, String->ByteArray", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.StringType, stackitem.ByteArrayT) // Special case, runtime.Notify will convert any Buffer to ByteArray.
+	})
+	t.Run("good, single type, PublicKeyType->ByteArray", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.PublicKeyType, stackitem.ByteArrayT)
+	})
+	t.Run("good, single type, AnyType->do not change initial type", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.AnyType, stackitem.IntegerT) // Special case, compiler should leave the type "as is" and do not emit conversion code.
+	})
+	// Test for InteropInterface->... is missing, because we don't enforce conversion to stackitem.InteropInterface,
+	// but compiler still checks these notifications against expected manifest.
+	t.Run("good, multiple types, check the conversion order", func(t *testing.T) {
+		check(t, methodWithoutEllipsis, []smartcontract.ParamType{
+			smartcontract.IntegerType,
+			smartcontract.BoolType,
+			smartcontract.ByteArrayType,
+			smartcontract.PublicKeyType,
+			smartcontract.Hash160Type,
+			smartcontract.AnyType, // leave initial type
+			smartcontract.StringType,
+		}, []stackitem.Type{
+			stackitem.IntegerT,
+			stackitem.BooleanT,
+			stackitem.ByteArrayT,
+			stackitem.ByteArrayT,
+			stackitem.ByteArrayT,
+			stackitem.IntegerT, // leave initial type
+			stackitem.ByteArrayT,
+		}, false)
+	})
+	t.Run("with ellipsis, do not emit conversion code", func(t *testing.T) {
+		checkSingleType(t, methodWithEllipsis, smartcontract.IntegerType, stackitem.IntegerT)
+		checkSingleType(t, methodWithEllipsis, smartcontract.BoolType, stackitem.IntegerT)
+		checkSingleType(t, methodWithEllipsis, smartcontract.ByteArrayType, stackitem.IntegerT)
+	})
+	t.Run("no events check => no conversion code", func(t *testing.T) {
+		checkSingleType(t, methodWithoutEllipsis, smartcontract.PublicKeyType, stackitem.IntegerT, true)
+	})
+}

pkg/smartcontract/param_type.go

@@ -331,3 +331,38 @@ func ConvertToParamType(val int) (ParamType, error) {
 	}
 	return UnknownType, errors.New("unknown parameter type")
 }
+
+// ConvertToStackitemType converts ParamType to corresponding Stackitem.Type.
+func (pt ParamType) ConvertToStackitemType() stackitem.Type {
+	switch pt {
+	case SignatureType:
+		return stackitem.ByteArrayT
+	case BoolType:
+		return stackitem.BooleanT
+	case IntegerType:
+		return stackitem.IntegerT
+	case Hash160Type:
+		return stackitem.ByteArrayT
+	case Hash256Type:
+		return stackitem.ByteArrayT
+	case ByteArrayType:
+		return stackitem.ByteArrayT
+	case PublicKeyType:
+		return stackitem.ByteArrayT
+	case StringType:
+		// Do not use BufferT to match System.Runtime.Notify conversion rules.
+		return stackitem.ByteArrayT
+	case ArrayType:
+		return stackitem.ArrayT
+	case MapType:
+		return stackitem.MapT
+	case InteropInterfaceType:
+		return stackitem.InteropT
+	case VoidType:
+		return stackitem.AnyT
+	case AnyType:
+		return stackitem.AnyT
+	default:
+		panic(fmt.Sprintf("unknown param type %d", pt))
+	}
+}

pkg/smartcontract/param_type_test.go

@@ -393,3 +393,28 @@ func TestConvertToParamType(t *testing.T) {
 	_, err := ConvertToParamType(0x01)
 	require.NotNil(t, err)
 }
+
+func TestConvertToStackitemType(t *testing.T) {
+	for p, expected := range map[ParamType]stackitem.Type{
+		AnyType:              stackitem.AnyT,
+		BoolType:             stackitem.BooleanT,
+		IntegerType:          stackitem.IntegerT,
+		ByteArrayType:        stackitem.ByteArrayT,
+		StringType:           stackitem.ByteArrayT,
+		Hash160Type:          stackitem.ByteArrayT,
+		Hash256Type:          stackitem.ByteArrayT,
+		PublicKeyType:        stackitem.ByteArrayT,
+		SignatureType:        stackitem.ByteArrayT,
+		ArrayType:            stackitem.ArrayT,
+		MapType:              stackitem.MapT,
+		InteropInterfaceType: stackitem.InteropT,
+		VoidType:             stackitem.AnyT,
+	} {
+		actual := p.ConvertToStackitemType()
+		require.Equal(t, expected, actual)
+	}
+
+	require.Panics(t, func() {
+		UnknownType.ConvertToStackitemType()
+	})
+}