Cross-contract exception constraints

[AnnaShaleva]

Sorry for raising this issue one more time, but we ned to clarify one thing.

Previous discussions

Consider the situation described in https://github.com/neo-project/neo-vm/pull/457#issuecomment-1119967254: contract A throws exception containing its storage context. Contract B catches the exception and retrieves A's storage context from it. After that contract B can modify A's storage.

https://github.com/neo-project/neo-vm/pull/457#issuecomment-1120159529 says that it's not a good idea to keep such option. I agree with that, it may be insecure, even if contract A throws own storage context by itself.

However, https://github.com/neo-project/neo-vm/pull/457 was closed and replaced by https://github.com/neo-project/neo/pull/2729. https://github.com/neo-project/neo-vm/pull/457#issuecomment-1123465184 says that snapshot isolation solves the described problem with leaking storage context.

The question

Could you, please, clarify, how snapshot isolation prevents contract B from catching A's storage context and putting malicious data inside it?

I wrote one more short test in Golang. It illustrates that changes of A's storage made by B during A's exception handling are persisted and reachable from the subsequent transactions. If C# node doesn't allow this, then neo-go implementation of snapshot isolation should be fixed.

func TestSnapshotIsolation_CatchAndAttack(t *testing.T) {
	bc, acc := chain.NewSingle(t)
	e := neotest.NewExecutor(t, bc, acc, acc)

	// Contract A has two methods. One of them panics and throws storage context as exception. Another method checks whether storage has value stored by `key`.
	srcA := `package contractA
		import "github.com/nspcc-dev/neo-go/pkg/interop/storage"
		func Panic() {
			ctx := storage.GetContext()
			panic(ctx)
		}
		func Check() {
			ctx := storage.GetContext()
			val := storage.Get(ctx, []byte("key"))
			if val != nil {
				panic("storage was modified by another contract")
			}
		}`
	ctrA := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(srcA), &compiler.Options{
		NoEventsCheck:      true,
		NoPermissionsCheck: true,
		Name:               "contractA",
	})
	e.DeployContract(t, ctrA, nil)

	var hashAStr string
	for i := 0; i < util.Uint160Size; i++ {
		hashAStr += fmt.Sprintf("%#x", ctrA.Hash[i])
		if i != util.Uint160Size-1 {
			hashAStr += ", "
		}
	}
	// Contract B calls A in try-catch block and modifies A's storage.
	srcB := `package contractB
		import (
			"github.com/nspcc-dev/neo-go/pkg/interop"
			"github.com/nspcc-dev/neo-go/pkg/interop/contract"
			"github.com/nspcc-dev/neo-go/pkg/interop/storage"
		)
		func CallAInTryCatch() {
			defer func() {
				if r := recover(); r != nil {
					ctx := r.(storage.Context)
					storage.Put(ctx, []byte("key"), []byte("value"))
				}
			}()
			contract.Call(interop.Hash160{` + hashAStr + `}, "panic", contract.All)
		}`
	ctrB := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(srcB), &compiler.Options{
		Name:               "contractB",
		NoEventsCheck:      true,
		NoPermissionsCheck: true,
		Permissions: []manifest.Permission{
			{
				Methods: manifest.WildStrings{Value: nil},
			},
		},
	})
	e.DeployContract(t, ctrB, nil)

	ctrBInvoker := e.NewInvoker(ctrB.Hash, e.Committee)
	ctrBInvoker.Invoke(t, stackitem.Null{}, "callAInTryCatch")

	ctrAInvoker := e.NewInvoker(ctrA.Hash, e.Committee)
	ctrAInvoker.Invoke(t, stackitem.Null{}, "check") // panic here.
}

[erikzhang]

Could you, please, clarify, how snapshot isolation prevents contract B from catching A's storage context and putting malicious data inside it?

Snapshot isolation doesn't prevent this situation. In fact we don't need to prevent it. Because the object is thrown by contract.

panic(ctx)

I don't know why, but it is the logic of the contract. Why do we disallow it? And even we disallowed it, it still can pass the StorageContext by returning.

return ctx;

[AnnaShaleva]

Why do we disallow it?

This behaviour is kind of not I'd expect from THROW, the contract developer may throw storage context by mistake. But if it's done intentionally, then probably it's OK.

@roman-khimov, @shargon, @devhawk what do you think about it?

[shargon]

Agree it's not solved, I prefer to limit the THROW item, but if we need to return an object, I don't know why, is not possible