Contract call arguments are not type-checked against contract's manifest

[roman-khimov]

Describe the bug
Contract's manifest has methods descriptions that among other things include a list of parameters. These parameters are typed, that is each parameter has a type associated with it. #1695 added a check for number of arguments to be equal the number of parameters, but there is no type checking being done. Missing type checks can lead to unpredictable behavior in called contract.

To Reproduce
Create a contract with method X that accepts some integer. Then call this method passing struct, array, interop interface and whatever else you want to it.

Expected behavior
Invocation should fail if argument types don't match parameter types.

[shargon]

The execution will fault anyway.

[roman-khimov]

It's not guaranteed to fail, it completely depends on contract's code and IMO lack of this check is a potential attack vector.

[Qiao-Jin]

potential attack vector

How?

[roman-khimov]

Sorry, I don't have PoC for you, but think of things like [1] --- in its essence it's an input data validation problem (like many others) and it's directly related to this particular bug, contract's authors specifying that they expect a Hash160 as a parameter may also expect that the value they'll receive really is Hash160, while in fact it could be anything and then the contract would do something unexpected with it. But if we're to type-check values before invoking the contract this attack wouldn't be possible.

1. https://www.apriorit.com/dev-blog/571-neo-nep-5-vulnerabilities

[Qiao-Jin]

the contract would do something unexpected with it

Wrong input argument types will give rise to problems such as errors & exceptions which will lead to vm FAULT.

[shargon]

I think that there are dangerous cases, but in my opinion, this cases should be checked by the smartcontract, if the code it's vulnerable without the neoVM (C#), it's the user responsibility.

[roman-khimov]

But the trick is exactly that it can be safe as pure C# (Go/Java/whatever) code because of the language type safety properties. And then you run it in Neo VM and suddenly there are no guarantees at contract method's boundaries.

[ixje]

Can we re-open this? This is the perfect example of wrong type with no VM failure neo-project/neo-devpack-dotnet#769 that could have been prevented

[mfbz]

I completely agree with this issue and think that we need a way to enforce type check at contract level. As an example, after 3.6.0 it happened that some contracts broke because from the client side it was pushed an integer as param in a method that accepted a ByteString, resulting in throwing an error when the event was fired with a ByteString param. This was a problem difficult to discover because from the contract it seemed all ok, while it was the client that was creating the error.

If we automatically check that the types of input params match method params types we could prevent these problems and possible exploits at contracts level. It can be thrown an exception and that would be easily discoverable from the dev.

[roman-khimov]

The only problem is that this was submitted Aug 31, 2020 and we're at 2023 with two years of running mainnet. This is very likely to affect already deployed contracts in a way similar to #2810. But this needs to be checked and hopefully there is an upgrade path.