Goal: Warn on invalid method signature assembly (#3614)

* Warn on invalid method signature assembly

* Print warnings to stderr

cmd/goal/account.go

@@ -396,7 +396,7 @@ var newMultisigCmd = &cobra.Command{
 			}
 		}
 		if duplicatesDetected {
-			reportWarnln(warnMultisigDuplicatesDetected)
+			reportWarnRawln(warnMultisigDuplicatesDetected)
 		}
 		// Generate a new address in the default wallet
 		addr, err := client.CreateMultisigAccount(wh, threshold, args)

cmd/goal/application.go

@@ -408,13 +408,13 @@ func mustParseProgArgs() (approval []byte, clear []byte) {
 	}
 
 	if approvalProgFile != "" {
-		approval = assembleFile(approvalProgFile)
+		approval = assembleFile(approvalProgFile, false)
 	} else {
 		approval = mustReadFile(approvalProgRawFile)
 	}
 
 	if clearProgFile != "" {
-		clear = assembleFile(clearProgFile)
+		clear = assembleFile(clearProgFile, false)
 	} else {
 		clear = mustReadFile(clearProgRawFile)
 	}
@@ -1238,7 +1238,7 @@ var methodAppCmd = &cobra.Command{
 		}
 
 		var retType *abi.Type
-		if retTypeStr != "void" {
+		if retTypeStr != abi.VoidReturnType {
 			theRetType, err := abi.TypeOf(retTypeStr)
 			if err != nil {
 				reportErrorf("cannot cast %s to abi type: %v", retTypeStr, err)

cmd/goal/clerk.go

@@ -331,7 +331,7 @@ var sendCmd = &cobra.Command{
 			if logicSigFile != "" {
 				reportErrorln("should use at most one of --from-program/-F or --from-program-bytes/-P --logic-sig/-L")
 			}
-			program = assembleFile(programSource)
+			program = assembleFile(programSource, false)
 		} else if logicSigFile != "" {
 			lsigFromArgs(&lsig)
 		}
@@ -733,7 +733,7 @@ var signCmd = &cobra.Command{
 			if logicSigFile != "" {
 				reportErrorln("goal clerk sign should have at most one of --program/-p or --logic-sig/-L")
 			}
-			lsig.Logic = assembleFile(programSource)
+			lsig.Logic = assembleFile(programSource, false)
 			lsig.Args = getProgramArgs()
 		} else if logicSigFile != "" {
 			lsigFromArgs(&lsig)
@@ -927,7 +927,7 @@ func mustReadFile(fname string) []byte {
 	return contents
 }
 
-func assembleFile(fname string) (program []byte) {
+func assembleFile(fname string, printWarnings bool) (program []byte) {
 	text, err := readFile(fname)
 	if err != nil {
 		reportErrorf("%s: %s", fname, err)
@@ -948,6 +948,17 @@ func assembleFile(fname string) (program []byte) {
 		}
 	}
 
+	if printWarnings && len(ops.Warnings) != 0 {
+		for _, warning := range ops.Warnings {
+			reportWarnRawln(warning.Error())
+		}
+		plural := "s"
+		if len(ops.Warnings) == 1 {
+			plural = ""
+		}
+		reportWarnRawf("%d warning%s", len(ops.Warnings), plural)
+	}
+
 	return ops.Program
 }
 
@@ -997,8 +1008,6 @@ var compileCmd = &cobra.Command{
 				disassembleFile(fname, outFilename)
 				continue
 			}
-			program := assembleFile(fname)
-			outblob := program
 			outname := outFilename
 			if outname == "" {
 				if fname == stdinFileNameValue {
@@ -1007,6 +1016,9 @@ var compileCmd = &cobra.Command{
 					outname = fmt.Sprintf("%s.tok", fname)
 				}
 			}
+			shouldPrintAdditionalInfo := outname != stdoutFilenameValue
+			program := assembleFile(fname, true)
+			outblob := program
 			if signProgram {
 				dataDir := ensureSingleDataDir()
 				accountList := makeAccountsList(dataDir)
@@ -1036,7 +1048,7 @@ var compileCmd = &cobra.Command{
 					reportErrorf("%s: %s", outname, err)
 				}
 			}
-			if !signProgram && outname != stdoutFilenameValue {
+			if !signProgram && shouldPrintAdditionalInfo {
 				pd := logic.HashProgram(program)
 				addr := basics.Address(pd)
 				fmt.Printf("%s: %s\n", fname, addr.String())

cmd/goal/commands.go

@@ -517,19 +517,33 @@ func reportInfof(format string, args ...interface{}) {
 	reportInfoln(fmt.Sprintf(format, args...))
 }
 
-func reportWarnln(args ...interface{}) {
-	fmt.Print("Warning: ")
-
+// reportWarnRawln prints a warning message to stderr. Only use this function if that warning
+// message already indicates that it's a warning. Otherwise, use reportWarnln
+func reportWarnRawln(args ...interface{}) {
 	for _, line := range strings.Split(fmt.Sprint(args...), "\n") {
 		printable, line := unicodePrintable(line)
 		if !printable {
-			fmt.Println(infoNonPrintableCharacters)
+			fmt.Fprintln(os.Stderr, infoNonPrintableCharacters)
 		}
 
-		fmt.Println(line)
+		fmt.Fprintln(os.Stderr, line)
 	}
 }
 
+// reportWarnRawf prints a warning message to stderr. Only use this function if that warning message
+// already indicates that it's a warning. Otherwise, use reportWarnf
+func reportWarnRawf(format string, args ...interface{}) {
+	reportWarnRawln(fmt.Sprintf(format, args...))
+}
+
+// reportWarnln prints a warning message to stderr. The message will be prefixed with "Warning: ".
+// If you don't want this prefix, use reportWarnRawln
+func reportWarnln(args ...interface{}) {
+	reportWarnRawf("Warning: %s", fmt.Sprint(args...))
+}
+
+// reportWarnf prints a warning message to stderr. The message will be prefixed with "Warning: ". If
+// you don't want this prefix, use reportWarnRawf
 func reportWarnf(format string, args ...interface{}) {
 	reportWarnln(fmt.Sprintf(format, args...))
 }

cmd/goal/logging.go

@@ -50,7 +50,7 @@ var loggingCmd = &cobra.Command{
 	Long:  `Enable/disable and configure Algorand remote logging.`,
 	Args:  validateNoPosArgsFn,
 	Run: func(cmd *cobra.Command, _ []string) {
-		fmt.Fprintf(os.Stderr, "Warning: `goal logging` deprecated, use `diagcfg telemetry status`\n")
+		reportWarnln("`goal logging` deprecated, use `diagcfg telemetry status`")
 		dataDir := ensureSingleDataDir()
 		cfg, err := logging.EnsureTelemetryConfig(&dataDir, "")
 
@@ -72,7 +72,7 @@ var enableCmd = &cobra.Command{
 	Long:  `This will turn on remote logging. The "friendly name" for the node, used by logging, will be determined by -n nodename.`,
 	Args:  validateNoPosArgsFn,
 	Run: func(cmd *cobra.Command, _ []string) {
-		fmt.Fprintf(os.Stderr, "Warning: `goal logging enable` deprecated, use `diagcfg telemetry enable`\n")
+		reportWarnln("`goal logging enable` deprecated, use `diagcfg telemetry enable`")
 		dataDir := ensureSingleDataDir()
 		cfg, err := logging.EnsureTelemetryConfig(&dataDir, "")
 		if err != nil {
@@ -93,7 +93,7 @@ var disableCmd = &cobra.Command{
 	Short: "Disable Algorand remote logging",
 	Args:  validateNoPosArgsFn,
 	Run: func(cmd *cobra.Command, _ []string) {
-		fmt.Fprintf(os.Stderr, "Warning: `goal logging disable` deprecated, use `diagcfg telemetry disable`\n")
+		reportWarnf("`goal logging disable` deprecated, use `diagcfg telemetry disable`")
 		dataDir := ensureSingleDataDir()
 		cfg, err := logging.EnsureTelemetryConfig(&dataDir, "")
 

data/abi/abi_encode.go

@@ -572,7 +572,12 @@ func ParseArgJSONtoByteSlice(argTypes []string, jsonArgs []string, applicationAr
 func ParseMethodSignature(methodSig string) (name string, argTypes []string, returnType string, err error) {
 	argsStart := strings.Index(methodSig, "(")
 	if argsStart == -1 {
-		err = fmt.Errorf("Invalid method signature: %s", methodSig)
+		err = fmt.Errorf(`No parenthesis in method signature: "%s"`, methodSig)
+		return
+	}
+
+	if argsStart == 0 {
+		err = fmt.Errorf(`Method signature has no name: "%s"`, methodSig)
 		return
 	}
 
@@ -583,7 +588,7 @@ func ParseMethodSignature(methodSig string) (name string, argTypes []string, ret
 			depth++
 		} else if char == ')' {
 			if depth == 0 {
-				err = fmt.Errorf("Unpaired parenthesis in method signature: %s", methodSig)
+				err = fmt.Errorf(`Unpaired parenthesis in method signature: "%s"`, methodSig)
 				return
 			}
 			depth--
@@ -595,7 +600,7 @@ func ParseMethodSignature(methodSig string) (name string, argTypes []string, ret
 	}
 
 	if argsEnd == -1 {
-		err = fmt.Errorf("Invalid method signature: %s", methodSig)
+		err = fmt.Errorf(`Unpaired parenthesis in method signature: "%s"`, methodSig)
 		return
 	}
 
@@ -604,3 +609,31 @@ func ParseMethodSignature(methodSig string) (name string, argTypes []string, ret
 	returnType = methodSig[argsEnd+1:]
 	return
 }
+
+// VerifyMethodSignature checks if a method signature and its referenced types can be parsed properly
+func VerifyMethodSignature(methodSig string) error {
+	_, argTypes, retType, err := ParseMethodSignature(methodSig)
+	if err != nil {
+		return err
+	}
+
+	for i, argType := range argTypes {
+		if IsReferenceType(argType) || IsTransactionType(argType) {
+			continue
+		}
+
+		_, err = TypeOf(argType)
+		if err != nil {
+			return fmt.Errorf("Error parsing argument type at index %d: %s", i, err.Error())
+		}
+	}
+
+	if retType != VoidReturnType {
+		_, err = TypeOf(retType)
+		if err != nil {
+			return fmt.Errorf("Error parsing return type: %s", err.Error())
+		}
+	}
+
+	return nil
+}

data/abi/abi_type.go

@@ -136,7 +136,7 @@ func TypeOf(str string) (Type, error) {
 		stringMatches := staticArrayRegexp.FindStringSubmatch(str)
 		// match the string itself, array element type, then array length
 		if len(stringMatches) != 3 {
-			return Type{}, fmt.Errorf("static array ill formated: %s", str)
+			return Type{}, fmt.Errorf(`static array ill formated: "%s"`, str)
 		}
 		// guaranteed that the length of array is existing
 		arrayLengthStr := stringMatches[2]
@@ -154,7 +154,7 @@ func TypeOf(str string) (Type, error) {
 	case strings.HasPrefix(str, "uint"):
 		typeSize, err := strconv.ParseUint(str[4:], 10, 16)
 		if err != nil {
-			return Type{}, fmt.Errorf("ill formed uint type: %s", str)
+			return Type{}, fmt.Errorf(`ill formed uint type: "%s"`, str)
 		}
 		return makeUintType(int(typeSize))
 	case str == "byte":
@@ -163,7 +163,7 @@ func TypeOf(str string) (Type, error) {
 		stringMatches := ufixedRegexp.FindStringSubmatch(str)
 		// match string itself, then type-bitSize, and type-precision
 		if len(stringMatches) != 3 {
-			return Type{}, fmt.Errorf("ill formed ufixed type: %s", str)
+			return Type{}, fmt.Errorf(`ill formed ufixed type: "%s"`, str)
 		}
 		// guaranteed that there are 2 uint strings in ufixed string
 		ufixedSize, err := strconv.ParseUint(stringMatches[1], 10, 16)
@@ -196,7 +196,7 @@ func TypeOf(str string) (Type, error) {
 		}
 		return MakeTupleType(tupleTypes)
 	default:
-		return Type{}, fmt.Errorf("cannot convert a string %s to an ABI type", str)
+		return Type{}, fmt.Errorf(`cannot convert the string "%s" to an ABI type`, str)
 	}
 }
 
@@ -493,3 +493,6 @@ func IsReferenceType(s string) bool {
 		return false
 	}
 }
+
+// VoidReturnType is the ABI return type string for a method that does not return any value
+const VoidReturnType = "void"

data/transactions/logic/assembler.go

@@ -31,6 +31,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/algorand/go-algorand/data/abi"
 	"github.com/algorand/go-algorand/data/basics"
 )
 
@@ -650,11 +651,18 @@ func asmMethod(ops *OpStream, spec *OpSpec, args []string) error {
 	}
 	arg := args[0]
 	if len(arg) > 1 && arg[0] == '"' && arg[len(arg)-1] == '"' {
-		val, err := parseStringLiteral(arg)
+		methodSig, err := parseStringLiteral(arg)
 		if err != nil {
 			return ops.error(err)
 		}
-		hash := sha512.Sum512_256(val)
+		methodSigStr := string(methodSig)
+		err = abi.VerifyMethodSignature(methodSigStr)
+		if err != nil {
+			// Warn if an invalid signature is used. Don't return an error, since the ABI is not
+			// governed by the core protocol, so there may be changes to it that we don't know about
+			ops.warnf("Invalid ARC-4 ABI method signature for method op: %s", err.Error()) // nolint:errcheck
+		}
+		hash := sha512.Sum512_256(methodSig)
 		ops.ByteLiteral(hash[0:4])
 		return nil
 	}

data/transactions/logic/assembler_test.go

@@ -2207,6 +2207,52 @@ func TestErrShortBytecblock(t *testing.T) {
 	require.Equal(t, err, errShortIntcblock)
 }
 
+func TestMethodWarning(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	tests := []struct {
+		method string
+		pass   bool
+	}{
+		{
+			method: "abc(uint64)void",
+			pass:   true,
+		},
+		{
+			method: "abc(uint64)",
+			pass:   false,
+		},
+		{
+			method: "abc(uint65)void",
+			pass:   false,
+		},
+		{
+			method: "(uint64)void",
+			pass:   false,
+		},
+		{
+			method: "abc(uint65,void",
+			pass:   false,
+		},
+	}
+
+	for _, test := range tests {
+		for v := uint64(1); v <= AssemblerMaxVersion; v++ {
+			src := fmt.Sprintf("method \"%s\"\nint 1", test.method)
+			ops, err := AssembleStringWithVersion(src, v)
+			require.NoError(t, err)
+
+			if test.pass {
+				require.Len(t, ops.Warnings, 0)
+				continue
+			}
+
+			require.Len(t, ops.Warnings, 1)
+			require.Contains(t, ops.Warnings[0].Error(), "Invalid ARC-4 ABI method signature for method op")
+		}
+	}
+}
+
 func TestBranchAssemblyTypeCheck(t *testing.T) {
 	partitiontest.PartitionTest(t)