SmartContract: catch exception on multisignature contract parsing

[AnnaShaleva]

Description

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.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Unit test TestIsMultiSigContract_WrongCurve
• Unit test TestIsSignatureContract_WrongCurve

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[vncoelho]

I think it is not "(non-breaking change which fixes an issue)", @AnnaShaleva .

Perhaps that without a proper version control an attacker could try to use this function and change behavior of past executions.
But I did not test yet.

[cschuchardt88]

Please write test for IVerifable.VerifyStateIndependent and IVeriable.VerifyWitnesses just to make sure they work with changes.

neo/src/Neo/Network/P2P/Payloads/Transaction.cs

Line 432 in 429a081

else if (IsMultiSigContract(witnesses[i].VerificationScript.Span, out var m, out ECPoint[] points))

neo/src/Neo/SmartContract/Helper.cs

Line 283 in 429a081

public static bool VerifyWitnesses(this IVerifiable verifiable, ProtocolSettings settings, DataCache snapshot, long gas)

[vncoelho]

@AnnaShaleva,

I double checked here,
perhaps it does not need version control. In fact, it looks like it is just internal for fee calculations and transaction creation. Thus, it would not affect the understanding of previous transactions.

[roman-khimov]

Correct fix (and likely more important than just to "properly handle non-Secp256r1-based witness scripts"), but I fear we may have more cases like this in the codebase (like VerifySignature).

[cschuchardt88]

@roman-khimov

Correct fix (and likely more important than just to "properly handle non-Secp256r1-based witness scripts"), but I fear we may have more cases like this in the codebase (like VerifySignature).

Hints my tests for IVerifable.VerifyStateIndependent if it errors it should return VerifyResult.Succeed.

#3211 (review)