Limit notifications

[erikzhang]

Added some restrictions to System.Runtime.Notify.

1. Recursive references in notifications are not allowed.
2. Sending Buffer, InteropInterface and Pointer in notifications is not allowed.

Notifications are deep copied as immutable.

[roman-khimov]

I don't think Buffer should be forbidden, it's very close to ByteString in its essence and depending on the compiler can be used in various circumstances. Notifications are size-limited anyway, so having some 32-byte Buffer there doesn't differ much from 32-byte ByteString.

[erikzhang]

To execute Notify we need to do a DeepCopy, to execute GetNotifications we need to do a DeepCopy again. I think allowing Buffer will severely impact performance and lead to attacks. The contract can easily convert Buffer to ByteString through a CONVERT instruction.

[roman-khimov]

The copy is only done in SendNotification(), so if the buffer is huge we won't get there anyway because of MaxNotificationSize limitation. So if any buffer is emitted, it won't be large and I doubt it would make any difference from ByteString of the same size. I mostly fear that contracts can emit buffers without knowing it, it all totally depends on the compiler in absence of #1879.

[erikzhang]

You need to DeepCopy when executing GetNotifications. Otherwise, the contract who received the notifications can modify them. One attack mode is to send a few hundred notifications containing Buffers and then call GetNotifications to force automatic DeepCopy of all Buffers each time.

[roman-khimov]

You need to DeepCopy when executing GetNotifications

Yep, #1707.

force automatic DeepCopy of all Buffers

Glancing over VM code I see that ByteString is not really copied during DeepCopy, so yes, there is a difference. Maybe if we charge per-element in #2748 that'd be irrelevant, though.

[shargon]

Wait for #2748, maybe in #2748 we should increase System.Runtime.Notify too

[erikzhang]

Wait for neo-project/neo-vm#473

[erikzhang]

Merge?