SmartContract: catch exception on multisignature contract parsing (#3211

)

IsMultiSigContract should return proper true/false result even if some
garbage is provided as an input. Without this commit an exception is
possible during IsMultiSigContract execution during subsequent public
key decoding from compressed form:
```
  Failed TestIsMultiSigContract [16 ms]
  Error Message:
   Test method Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract threw exception:
System.FormatException: Invalid point encoding 221
  Stack Trace:
      at Neo.Cryptography.ECC.ECPoint.DecodePoint(ReadOnlySpan`1 encoded, ECCurve curve) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/Cryptography/ECC/ECPoint.cs:line 98
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script, Int32& m, Int32& n, List`1 points) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 189
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 123
   at Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract() in /home/anna/Documents/GitProjects/neo-project/neo/tests/Neo.UnitTests/SmartContract/UT_Helper.cs:line 65
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
```

Note that this change is compatible wrt the callers' behaviour. We need
this change to properly handle non-Secp256r1-based witness scripts, ref.
#3209.

Signed-off-by: Anna Shaleva <[email protected]>

src/Neo/SmartContract/Helper.cs

@@ -184,7 +184,14 @@ private static bool IsMultiSigContract(ReadOnlySpan<byte> script, out int m, out
             {
                 if (script.Length <= i + 35) return false;
                 if (script[++i] != 33) return false;
-                points?.Add(ECPoint.DecodePoint(script.Slice(i + 1, 33), ECCurve.Secp256r1));
+                try
+                {
+                    points?.Add(ECPoint.DecodePoint(script.Slice(i + 1, 33), ECCurve.Secp256r1));
+                }
+                catch (Exception) // Script may contain any data, thus exceptions are allowed on point decoding.
+                {
+                    return false;
+                }
                 i += 34;
                 ++n;
             }

tests/Neo.UnitTests/SmartContract/UT_Helper.cs

@@ -17,6 +17,8 @@
 using Neo.VM;
 using Neo.Wallets;
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using static Neo.SmartContract.Helper;
 
 namespace Neo.UnitTests.SmartContract
@@ -72,6 +74,47 @@ public void TestIsMultiSigContract()
             Assert.IsFalse(IsMultiSigContract(case2));
         }
 
+        [TestMethod]
+        // TestIsMultiSigContract_WrongCurve checks that multisignature verification script based on points
+        // not from Secp256r1 curve fails IsMultiSigContract check without any exception.
+        public void TestIsMultiSigContract_WrongCurve()
+        {
+            // A set of points on Koblitz curve in uncompressed representation. One of the points
+            // (the first one) is specially selected, this point can't be restored on Secp256r1
+            // from compressed form, whereas three other points can be restored on both Secp256r1
+            // and Koblitz curves.
+            var pubs = new List<ECPoint>()
+            {
+                ECPoint.Parse("047b4e72ae854b6a0955b3e02d92651ab7fa641a936066776ad438f95bb674a269a63ff98544691663d91a6cfcd215831f01bfb7a226363a6c5c67ef14541dba07", ECCurve.Secp256k1),
+                ECPoint.Parse("040486468683c112125978ffe876245b2006bfe739aca8539b67335079262cb27ad0dedc9e5583f99b61c6f46bf80b97eaec3654b87add0e5bd7106c69922a229d", ECCurve.Secp256k1),
+                ECPoint.Parse("040d26fc2ad3b1aae20f040b5f83380670f8ef5c2b2ac921ba3bdd79fd0af0525177715fd4370b1012ddd10579698d186ab342c223da3e884ece9cab9b6638c7bb", ECCurve.Secp256k1),
+                ECPoint.Parse("04a114d72fe2997cdac67427b6f39ea08ed46213c8bb6a461bbac2a6212cf43fb510f8adf59b0b087a7859f96d0288e5e94800eab8388f30f03f92b2e4d807dfce", ECCurve.Secp256k1)
+            };
+            const int m = 3;
+
+            var badScript = Contract.CreateMultiSigRedeemScript(m, pubs);
+            Assert.IsFalse(IsMultiSigContract(badScript, out _, out ECPoint[] _)); // enforce runtime point decoding by specifying ECPoint[] out variable.
+            Assert.IsTrue(IsMultiSigContract(badScript)); // this overload is unlucky since it doesn't perform ECPoint decoding.
+
+            // Exclude the first special point and check one more time, both methods should return true.
+            var goodScript = Contract.CreateMultiSigRedeemScript(m, pubs.Skip(1).ToArray());
+            Assert.IsTrue(IsMultiSigContract(goodScript, out _, out ECPoint[] _)); // enforce runtime point decoding by specifying ECPoint[] out variable.
+            Assert.IsTrue(IsMultiSigContract(goodScript)); // this overload is unlucky since it doesn't perform ECPoint decoding.
+        }
+
+        [TestMethod]
+        // TestIsSignatureContract_WrongCurve checks that signature verification script based on point
+        // not from Secp256r1 curve passes IsSignatureContract check without any exception.
+        public void TestIsSignatureContract_WrongCurve()
+        {
+            // A special point on Koblitz curve that can't be restored at Secp256r1 from compressed form.
+            var pub = ECPoint.Parse("047b4e72ae854b6a0955b3e02d92651ab7fa641a936066776ad438f95bb674a269a63ff98544691663d91a6cfcd215831f01bfb7a226363a6c5c67ef14541dba07", ECCurve.Secp256k1);
+            var script = Contract.CreateSignatureRedeemScript(pub);
+
+            // IsSignatureContract should pass since it doesn't perform ECPoint decoding.
+            Assert.IsTrue(IsSignatureContract(script));
+        }
+
         [TestMethod]
         public void TestSignatureContractCost()
         {