neo-project
Contract call arguments are not type-checked against contract's manifest
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.
It's not guaranteed to fail, it completely depends on contract's code and IMO lack of this check is a potential attack vector.
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.
- https://www.apriorit.com/dev-blog/571-neo-nep-5-vulnerabilities
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.
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.
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.
Can we re-open this? This is the perfect example of wrong type with no VM failure https://github.com/neo-project/neo-devpack-dotnet/issues/769 that could have been prevented
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.
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.
Just previous week @carpawell spent some time debugging surprising behavior in the code using cross-contract calls. And the root cause of the problem (https://github.com/nspcc-dev/neofs-contract/pull/519) was exactly that there are no type checks performed when one contract calls another. Notice that in this case contract implementation language type system is useless, Contract.Call operates with an array of objects in C# much like in Go we accept any for the respective method. So anything can be passed regardless of what the called contract expects.
In this particular case there were no FAULTs, everything executed till the end (PICKITEM happily working with a byte string instead of a proper array is very handy). What's more interesting is that this case couldn't be prevented even if the check was implemented some years ago using NEP-14 data. An array of arrays of byte arrays is the same type as an array of byte arrays for NEP-14, but there is a difference in extended types NEP-25 provides, so my suggestion is that we need full NEP-25 checks here.
Its all ByteString in the end. Yes, Integer of 1 can be boolean true. The problem comes in working out a type of Any. That type should allow integer or whatever to be passed in. I think the it should stay the same, but change it to convert to the parameter type for the method, if that fails than fault vm.
