UInt160 can be instantiated with ReadOnlySpan<byte> of inappropriate size

[roman-khimov]

Describe the bug
There are no size checks in the UInt160 constructor accepting ReadOnlySpan<byte>:

neo/src/neo/UInt160.cs

Lines 48 to 59 in abd53e8

	/// <summary>
	/// Initializes a new instance of the <see cref="UInt160"/> class.
	/// </summary>
	/// <param name="value">The value of the <see cref="UInt160"/>.</param>
	public unsafe UInt160(ReadOnlySpan<byte> value)
	{
	fixed (ulong* p = &value1)
	{
	Span<byte> dst = new(p, Length);
	value[..Length].CopyTo(dst);
	}
	}

One can pass more than 20 bytes into it and it'll be successful. Notice that 2.x instantiated Uint160 via UintBase which had an explicit length check:

neo/neo/UIntBase.cs

Lines 30 to 40 in 0b6526e

	protected UIntBase(int bytes, byte[] value)
	{
	if (value == null)
	{
	this.data_bytes = new byte[bytes];
	return;
	}
	if (value.Length != bytes)
	throw new ArgumentException();
	this.data_bytes = value;
	}

Seems like the behavior was changed in 425dbd5.

To Reproduce
Absent this check we have transaction a53925381d2b0e38a6025c0a3a05076c4ccb57c46021fd47ee0a812f8c9da424 on testnet succeeding where it should fail. The contract invoked there is some kind of NEP-11 that seems to be broken, the entry script of the transaction looks like this:

NEO-GO-VM 0 > ops
INDEX    OPCODE       PARAMETER
0        PUSHDATA1    35 ("5")    <<
3        PUSHDATA1    34 ("4")
6        PUSHDATA1    33 ("3")
9        PUSHDATA1    32 ("2")
12       PUSHDATA1    31 ("1")
15       PUSHDATA1    307832323936626433323330303462343339663436643135353762376235386138663663666533366166 ("0x2296bd323004b439f46d1557b7b58a8f6cfe36af")
59       PUSH6        
60       PACK         
61       PUSH15       
62       PUSHDATA1    637265617465 ("create")
70       PUSHDATA1    a39b664c3fadde872b1e3b3b4518aadf7d54b69c
92       SYSCALL      System.Contract.Call (627d5b52)
97       PUSHDATA1    3535 ("55")
101      PUSHDATA1    3434 ("44")
105      PUSHDATA1    3333 ("33")
109      PUSHDATA1    3232 ("22")
113      PUSHDATA1    3131 ("11")
117      PUSHDATA1    307832323936626433323330303462343339663436643135353762376235386138663663666533366166 ("0x2296bd323004b439f46d1557b7b58a8f6cfe36af")
161      PUSH6        
162      PACK         
163      PUSH15       
164      PUSHDATA1    637265617465 ("create")
172      PUSHDATA1    a39b664c3fadde872b1e3b3b4518aadf7d54b69c
194      SYSCALL      System.Contract.Call (627d5b52)

create method is being called, but for some reason the address to be used (that corresponds to the sender address, btw) is passed as a 42-byte hex string instead of 20-byte raw binary. This string is then used as a destination address in the Transfer notification (broken by the NEP-11 definition of it, but still fine for System.Runtime.Notify) and as a parameter to the getContract ContractManagement's method. That's where the execution flow becomes a bit strange, because in C# node ContractManagement actually receives and processes this call with valid UInt160 (created by stripping excessive bytes).

Expected behavior
NeoGo refuses this call because the getContract parameter in this case is obviously not a valid Uint160 and I believe that's the correct behavior. At the moment it just hides a bug in the contract (invocation succeeds) and may lead to many more surprises in future.

Platform:

• OS: Any
• Version neo-cli 3.1.0

(Optional) Additional context
Related to nspcc-dev/neo-go#2337.

[roman-khimov]

This string is then used as a destination address in the Transfer notification (broken by the NEP-11 definition of it, but still fine for System.Runtime.Notify)

Which BTW reminds me of #1879, because the contract has Hash160 specified as Transfer event argument and the thing it emits is not a proper Hash160.

[Jim8y]

One can pass more than 20 bytes into it and it'll be successful.

So its not a StackOverflow vulnerability as the constructor only take the first 20 bytes to construct the new UInt160.

This may or may not be an issue just depends on how you wanna define the behavior.