Stricter contract checks inside Contract.Create and Contract.Update

[roman-khimov]

Summary or problem description
We do almost no checks for contracts and manifests deployed and I think it's a problem. At the moment one can deploy some /dev/urandom contents as a script with almost any garbage in the manifest (the only restriction is ABI hash and group signature correctness) and it will be happily accepted by the chain. I don't think it gives us any benefits, in fact it could be beneficial to provide a bit more guarantees to our users by stricter checks of contracts on their creation/update.

Do you have any solution you want to propose?
We have #1883 for one specific problem already, but I'd like to outline some other things we can check:

• opcodes for correctness (hi, @igormcoelho and Deploy parse script for integrity #733)
  Notice that there is a check for that implemented in Update deploy command following NEO3 schema neo-node#463, but IMO it's done in the wrong place.
• offsets in manifests for out of bounds conditions (hi, @csmuller and Neo-vm returns ANY when invoking a script at an address that is outside of its bounds neo-vm#376)
  Of course Neo-vm returns ANY when invoking a script at an address that is outside of its bounds neo-vm#376 should also be handled in the Contract.Call or VM to fail at that level, but I think it's not enough, incorrect manifest shouldn't be deployed in the first place.
• jumps and calls for offsets pointing to natural instructions boundaries (jumps into the middle of PUSHDATA4 are not good)
• manifested offsets for the same instruction-boundary validity
• manifested methods and events uniqueness
• manifested permissions for contract/method existence/validity
• safe methods for existence

It sure will add some overhead to these calls, but TPS metrics are absolutely irrelevant here IMO, because contract creation/update is non-frequent and costly operation. We can do that and this service is being paid for (hi, #2001 also). At the same time we will surely prevent some (not all, of course) errors and tricks that could be done and provide more predictable and safer execution environment.

Neo Version

• Neo 3

Where in the software does this update applies to?

• Ledger
• Syscalls

[shargon]

Thinking about it, I agree with @Qiao-Jin here (#1883 (comment)) , for example we can't check a valid script without executing the script.

PUSHDATA 0x2121212121212121212121212121212121212121212121212121212121212140
JMP -10

It's a valid script and you can't parse if it's valid without execute it.

[roman-khimov]

JMP offsets are actually trivial to check without executing them (it can even be done in the same single pass over the bytecode with opcode checks, though two passes make it a bit easier). But it's just one of many things.

[roman-khimov]

It's a valid script

I've probably missed the main point. Of course the check would say that this particular script is invalid and it's perfectly fine for me.

We accept some bytecode to store and execute in a public setting, I think it implies some responsibility for it, so if we can with reasonable effort prevent some errors and misbehavior we better do that. And Contract.Create/Update syscalls is just where it all happens, either contract (and manifest) gets accepted or not, that's why it has to be done there.

[shargon]

Validate only abi and manifest it's ok for me

[roman-khimov]

We can start with that, it's easier and it already adds some value.

[igormcoelho]

@shargon the point is having something syntatically correct, at least, which "your eyes" have done already in your example hahaha
But neovm doesn't see it that way. Example, is this script syntatically valid: 010203ab010809bc
Dont take the time to check please, I just put some random bytes. It could be valid or not, I dont know. But if thats invalid, meaning that I'm "using" opcodes that dont exist (yet), this code is certainly flawed for neovm, but it may still execute, give true or false (or FAULT). And in some future, it could be that some nonexisting opcode becomes "existing", and interpretation changes. Its even worse when one reads some prefix like NEF, but content doesnt make any sense (because its random). This simple thing can waste the time if many people that look at the chain.
Chain can be broken in other manners too, but this is quite unnaceptable, because people attaching random bytes as invocation can already cause us huge problems. Validation is different than verification/invocation, and it just takes linear time of the number of bytes in script.

[igormcoelho]

Before I forget... again, I think we should check the integrity of all scripts, even Verification/Witness scripts. If we do that, and we are certain that previous Neo2 scripts will fail on Neo3, then on Neo layer (not NeoVM), we can support some "if" case that allows mapping from previous standard Neo2 script (with CHECKSIG opcode) to equivalent standard Neo3 script. This does not cover all cases, but if it covers 99% of users, that's very good (specially those who may be "left behind" during migration). Again, this "if" would skip execution from Neo3-VM, and manually execute the check (21 + pubkey + ac).