From a6f52a7180c2bdfd26cbf4bc776066e89718bbad Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 15 Mar 2024 22:01:00 +0300 Subject: [PATCH] core: prohibit reentry to Notary withdraw If we're withdrawing funds to contract that has onNEP17Payment method, then it may call Notary's withdraw one more time, but the account's state is not yet updated by this moment. The problem is similar to https://github.com/neo-project/neo/pull/2734. Signed-off-by: Anna Shaleva --- pkg/core/native/native_test/notary_test.go | 78 ++++++++++++++++++++++ pkg/core/native/notary.go | 6 +- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/pkg/core/native/native_test/notary_test.go b/pkg/core/native/native_test/notary_test.go index 67d100cc6..c64ff18ae 100644 --- a/pkg/core/native/native_test/notary_test.go +++ b/pkg/core/native/native_test/notary_test.go @@ -3,8 +3,11 @@ package native_test import ( "math" "math/big" + "strings" "testing" + "github.com/nspcc-dev/neo-go/internal/random" + "github.com/nspcc-dev/neo-go/pkg/compiler" "github.com/nspcc-dev/neo-go/pkg/config" "github.com/nspcc-dev/neo-go/pkg/core/native/nativenames" "github.com/nspcc-dev/neo-go/pkg/core/native/noderoles" @@ -13,6 +16,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/neotest" "github.com/nspcc-dev/neo-go/pkg/neotest/chain" "github.com/nspcc-dev/neo-go/pkg/rpcclient/notary" + "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" "github.com/nspcc-dev/neo-go/pkg/util" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" @@ -160,6 +164,80 @@ func TestNotary_Pipeline(t *testing.T) { notaryCommitteeInvoker.Invoke(t, 5760+e.Chain.BlockHeight()-3, "expirationOf", accHash) } +func TestNotary_MaliciousWithdrawal(t *testing.T) { + const defaultDepositDeltaTill = 5760 + notaryCommitteeInvoker := newNotaryClient(t) + e := notaryCommitteeInvoker.Executor + gasCommitteeInvoker := e.CommitteeInvoker(e.NativeHash(t, nativenames.Gas)) + + notaryHash := notaryCommitteeInvoker.NativeHash(t, nativenames.Notary) + feePerKey := e.Chain.GetNotaryServiceFeePerKey() + multisigHash := notaryCommitteeInvoker.Validator.ScriptHash() // matches committee's one for single chain + + checkBalanceOf := func(t *testing.T, acc util.Uint160, expected int64) { // we don't have big numbers in this test, thus may use int + notaryCommitteeInvoker.CheckGASBalance(t, acc, big.NewInt(expected)) + } + + // Check Notary contract has no GAS on the account. + checkBalanceOf(t, notaryHash, 0) + + // Perform several deposits to a set of different accounts. + count := 3 + for i := 0; i < count; i++ { + h := random.Uint160() + gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, 2*feePerKey, ¬ary.OnNEP17PaymentData{Account: &h, Till: e.Chain.BlockHeight() + 2}) + } + checkBalanceOf(t, notaryHash, int64(count)*2*feePerKey) + + // Deploy malicious contract and make Notary deposit for it. + src := `package foo + import ( + "github.com/nspcc-dev/neo-go/pkg/interop/native/notary" + "github.com/nspcc-dev/neo-go/pkg/interop/runtime" + "github.com/nspcc-dev/neo-go/pkg/interop" + "github.com/nspcc-dev/neo-go/pkg/interop/storage" + ) + const ( + prefixMaxCallDepth = 0x01 + defaultCallDepth = 3 + ) + func _deploy(_ any, isUpdate bool) { + ctx := storage.GetContext() + storage.Put(ctx, prefixMaxCallDepth, defaultCallDepth) + } + func OnNEP17Payment(from interop.Hash160, amount int, data any) { + ctx := storage.GetContext() + depth := storage.Get(ctx, prefixMaxCallDepth).(int) + if depth > 0 { + storage.Put(ctx, prefixMaxCallDepth, depth-1) + notary.Withdraw(runtime.GetExecutingScriptHash(), runtime.GetExecutingScriptHash()) + } + } + func Verify() bool { + return true + }` + ctr := neotest.CompileSource(t, e.CommitteeHash, strings.NewReader(src), &compiler.Options{Name: "Helper", Permissions: []manifest.Permission{{ + Methods: manifest.WildStrings{}, + }}}) + e.DeployContract(t, ctr, nil) + gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, 2*feePerKey, ¬ary.OnNEP17PaymentData{Account: &ctr.Hash, Till: e.Chain.BlockHeight() + 2}) + checkBalanceOf(t, notaryHash, int64(count+1)*2*feePerKey) + depositLock := e.Chain.BlockHeight() + defaultDepositDeltaTill + + // Try to perform malicious withdrawal from Notary contract. In malicious scenario the withdrawal cycle + // will be triggered by the contract itself (i.e. by malicious caller of the malicious contract), but + // for the test simplicity we'll use committee account. + for e.Chain.BlockHeight() <= depositLock { // side account made Notary deposit for contract, thus, default deposit delta till is used. + e.AddNewBlock(t) + } + maliciousInvoker := e.NewInvoker(notaryHash, notaryCommitteeInvoker.Signers[0], neotest.NewContractSigner(ctr.Hash, func(tx *transaction.Transaction) []any { + return nil + })) + maliciousInvoker.Invoke(t, true, "withdraw", ctr.Hash, ctr.Hash) + checkBalanceOf(t, notaryHash, int64(count)*2*feePerKey) + gasCommitteeInvoker.CheckGASBalance(t, ctr.Hash, big.NewInt(2*feePerKey)) +} + func TestNotary_NotaryNodesReward(t *testing.T) { checkReward := func(nKeys int, nNotaryNodes int, spendFullDeposit bool) { notaryCommitteeInvoker := newNotaryClient(t) diff --git a/pkg/core/native/notary.go b/pkg/core/native/notary.go index 14d328893..ed60d3ff8 100644 --- a/pkg/core/native/notary.go +++ b/pkg/core/native/notary.go @@ -305,6 +305,11 @@ func (n *Notary) withdraw(ic *interop.Context, args []stackitem.Item) stackitem. if err != nil { panic(fmt.Errorf("failed to get GAS contract state: %w", err)) } + + // Remove deposit before GAS transfer processing to avoid double-withdrawal. + // In case if Gas contract call fails, state will be rolled back. + n.removeDepositFor(ic.DAO, from) + transferArgs := []stackitem.Item{stackitem.NewByteArray(n.Hash.BytesBE()), stackitem.NewByteArray(to.BytesBE()), stackitem.NewBigInteger(deposit.Amount), stackitem.Null{}} err = contract.CallFromNative(ic, n.Hash, cs, "transfer", transferArgs, true) if err != nil { @@ -313,7 +318,6 @@ func (n *Notary) withdraw(ic *interop.Context, args []stackitem.Item) stackitem. if !ic.VM.Estack().Pop().Bool() { panic("failed to transfer GAS from Notary account: `transfer` returned false") } - n.removeDepositFor(ic.DAO, from) return stackitem.NewBool(true) }