Neo: Allow user to disallow features on invocation transaction (storage, dynamic invoke, ...)

[igormcoelho]

I think the user should have the right to disallow contract features when invoking. For example, if I'm executing a simple function such as "GetName()", I don't want the contract to be able to write anything on storage (but read is ok). Or perhaps I don't want it to perform a dynamic invoke, or any other feature.
This is related to Issue on Feature Manifest https://github.com/neo-project/neo/issues/287, but with a different vision... the manifest will publicly show the contract options, but as they can be still very broad, I think the user should have the final power to invoke functions safely.
This could be implemented with an extra flag on invocation transaction ("safe" flag), or an extra byte with different options for different safety combinations (no storage read, no storage write, no dynamic invoke, ...).

[shargon]

I like it :)

[igormcoelho]

Ok then, lets discuss permissions :)
I'm thinking on a byte enum used on neo-vm and application engine with the following options:
0: all allowed
1: no storage write (no put)
2: no storage (block getstoragecontext, no put, no get)
4: no dynamicinvoke

In the future, we can add other interesting options, no jumps, no calls, no notifications... we are limited to 8 bits, but I guess these here can start something good.

[shargon]

we can use flags

AllAllowed=0x00
DenyStorage=0x01
DenyDynamicInvoke=0x02
...

[erikzhang]

We can add this feature, but it can be difficult for normal users to use.

[igormcoelho]

I agree Erik, it will be hard to use, but I give you one important use case: I think NEP5 should disable dynamicinvoke by default, for security reasons. Wallets can do that for users. It would be nice to put this info in your Feature Manifest proposal, but the issue is that old nep5 will receive a * flag, for backwards compatibility, what may be very open... I dont know exacly, some more voices may be useful here @canesin @vncoelho.

[jsolman]

It’s a good idea. It would be nice if an exchange contract could use it when doing a dynamic invoke so that it could be sure that the contract it is calling could use storage for example, but could not in turn do another dynamic invoke. This would probably allow a decentralized exchange contract to be written where users could trade any token by script hash without needing any whitelisting in the contract by a trusted party to ensure all contract held funds on behalf of users remain safe.

[ediopia]

Can anyone message me why dynamicinvoke has a security problem? My team launched a p2p dex using dynamicinvoke and I've checked the latest projects from COZ have blocked dynamicinvoke in their transfer method. Dynamicinvoke is useless without handling the nep5 transfer and should let devs know about it.

I like this idea if it can embrace dynamicinvokes and make users decide to allow it. Not banning it by the contracts.

[jsolman]

As understand it, the idea is for users and contracts to be able to call other contracts with dynamic invoke disabled if they choose. NEP5 contracts would still be allowed to be called with dynamic invoke if they support it; which most do in some manner; though a few don’t.

[belane]

@xxedocxx there is not a dynamicinvoke vulnerability itself, is more if an user calls malicious contracts without control.
I think this PR is not related with you are talking about, fell free to contact us on discord (same name) for these doubts.

[igormcoelho]

@shargon I think Storage Reads may never be an issue... but I've been talking to @belane, and perhaps user may want to disallow dynamic invoke and other external appcalls some times. Should we consider Disable External Calls (APPCALL/TAILCALL)?

AllAllowed=0x00
DenyStorageWrite=0x01
DenyExternalCalls=0x02
DenyDynamicInvoke=0x04 (including static appcall/tailcall)

[erikzhang]

Consider an invocation chain like this: InvocationTransaction -> ContractA -> ContractB.
We need a way to allow ContractA to write to the store and reject ContractB to write to the store.

[igormcoelho]

Good idea Erik, that will be very powerful! I think on two options:

1. pass a bytearray list with permissions that is consumed as invocations ho deeper. example: [0x00, 0x01]
   I think this is not very good, too flexible and complicated perhaps (spends too much space too).
2. Consider only a 2-level permission system with an extra flag 0x08 (still a single byte enum on total):
   AllAllowed=0x00
   DenyStorageWrite=0x01
   DenyExternalCalls=0x02
   DenyDynamicInvoke=0x04
   DenyOnSecondInvoke=0x08

So, the proposed situation would fit well: 0x08 + 0x01 = 0x09. Obviously, it would be not possible to combine everything, example, deny write on first but not second (I doubt to exist some application for it), but not all combinations would make sense, example deny external invoke on first and something on second level too. Do we need a third level? Perhaps, but we can add in the future only if necessary.

[igormcoelho]

@erikzhang I believe we can resolve this situation with a much simpler approach, which is very user-friendly: https://github.com/neo-project/neo/issues/544

Using contract-specific witnesses, User wallets could attach scripts only valid for the desired contracts on invocation. So, if an application requires that user signs a witness for two different scripts (because of dynamic invoke), user will become aware of that situation. From contract perspective, nothing will change (fully backwards compatible), because CheckWitness will simply work in a given contract and perhaps fail in a undesired surrogated contract (called by dyn invoke, for example) if ContractSpecificScript attribute is used instead of Script.

[igormcoelho]

I'm closing this in favor of https://github.com/neo-project/neo/issues/544, since that is much simpler and already solves the target issue.