From be902afe9e6b238c91a8bc0e42d530a5efcbd072 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 9 Jun 2021 11:58:58 +0300 Subject: [PATCH] core: do not allow NEP17 roundtrip in case of insufficient funds NEP17 roundtrip is prohibited if from account doesn't have enough funds. This commit fixes states diff in block 92057 where account NfuwpaQ1A2xaeVbxWe8FRtaRgaMa8yF3YM initiates two NEO roundtrips with amount exceeding the account's balance: block 92057: value mismatch for key +////xTbYWBH3r5qhRKZAPFPHabKfb2vhQ==: QQMhAkwBIQOZZwEA vs QQMhAkwBIQN/ZwEA block 92057: value mismatch for key +v///ws=: kqlddcitCg== vs tphddcitCg== block 92057: value mismatch for key +v///xTbYWBH3r5qhRKZAPFPHabKfb2vhQ==: QQEhBUWyDu0W vs QQEhBWmhDu0W C#'s applog (contains False and False on stack for both transfers): ``` { "id" : 1, "jsonrpc" : "2.0", "result" : { "executions" : [ { "gasconsumed" : "11955500", "exception" : null, "stack" : [ { "value" : false, "type" : "Boolean" }, { "value" : false, "type" : "Boolean" } ], "vmstate" : "HALT", "trigger" : "Application", "notifications" : [] } ], "txid" : "0x8e73a7e9a566a514813907272ad65fc965002c3b098eacc5bdda529af19d7688" } } ``` Go's applog (both transfers succeeded and GAS minted): ``` { "result" : { "executions" : [ { "gasconsumed" : "11955500", "trigger" : "Application", "stack" : [ { "type" : "Boolean", "value" : true }, { "type" : "Boolean", "value" : true } ], "vmstate" : "HALT", "notifications" : [ { "eventname" : "Transfer", "contract" : "0xd2a4cff31913016155e38e474a2c06d08be276cf", "state" : { "value" : [ { "type" : "Any" }, { "value" : "22FgR96+aoUSmQDxTx2myn29r4U=", "type" : "ByteString" }, { "value" : "4316", "type" : "Integer" } ], "type" : "Array" } }, { "state" : { "type" : "Array", "value" : [ { "value" : "22FgR96+aoUSmQDxTx2myn29r4U=", "type" : "ByteString" }, { "type" : "ByteString", "value" : "22FgR96+aoUSmQDxTx2myn29r4U=" }, { "type" : "Integer", "value" : "1111111111" } ] }, "contract" : "0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5", "eventname" : "Transfer" }, { "contract" : "0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5", "state" : { "type" : "Array", "value" : [ { "value" : "22FgR96+aoUSmQDxTx2myn29r4U=", "type" : "ByteString" }, { "type" : "ByteString", "value" : "22FgR96+aoUSmQDxTx2myn29r4U=" }, { "type" : "Integer", "value" : "1111111" } ] }, "eventname" : "Transfer" } ] } ], "txid" : "0x8e73a7e9a566a514813907272ad65fc965002c3b098eacc5bdda529af19d7688" }, "id" : 1, "jsonrpc" : "2.0" } ``` --- pkg/core/helper_test.go | 14 ++++++++-- pkg/core/native/native_nep17.go | 26 +++++++++++++++--- pkg/core/native_gas_test.go | 48 +++++++++++++++++++++++++++++++++ pkg/core/native_neo_test.go | 32 ++++++++++++++++++++++ 4 files changed, 115 insertions(+), 5 deletions(-) diff --git a/pkg/core/helper_test.go b/pkg/core/helper_test.go index 14bb9bc3e..227dce033 100644 --- a/pkg/core/helper_test.go +++ b/pkg/core/helper_test.go @@ -498,9 +498,15 @@ func initBasicChain(t *testing.T, bc *Blockchain) { } func newNEP17Transfer(sc, from, to util.Uint160, amount int64, additionalArgs ...interface{}) *transaction.Transaction { + return newNEP17TransferWithAssert(sc, from, to, amount, true, additionalArgs...) +} + +func newNEP17TransferWithAssert(sc, from, to util.Uint160, amount int64, needAssert bool, additionalArgs ...interface{}) *transaction.Transaction { w := io.NewBufBinWriter() emit.AppCall(w.BinWriter, sc, "transfer", callflag.All, from, to, amount, additionalArgs) - emit.Opcodes(w.BinWriter, opcode.ASSERT) + if needAssert { + emit.Opcodes(w.BinWriter, opcode.ASSERT) + } if w.Err != nil { panic(fmt.Errorf("failed to create nep17 transfer transaction: %w", w.Err)) } @@ -677,7 +683,11 @@ func transferTokenFromMultisigAccountCheckOK(t *testing.T, chain *Blockchain, to } func transferTokenFromMultisigAccount(t *testing.T, chain *Blockchain, to, tokenHash util.Uint160, amount int64, additionalArgs ...interface{}) *transaction.Transaction { - transferTx := newNEP17Transfer(tokenHash, testchain.MultisigScriptHash(), to, amount, additionalArgs...) + return transferTokenFromMultisigAccountWithAssert(t, chain, to, tokenHash, amount, true, additionalArgs...) +} + +func transferTokenFromMultisigAccountWithAssert(t *testing.T, chain *Blockchain, to, tokenHash util.Uint160, amount int64, needAssert bool, additionalArgs ...interface{}) *transaction.Transaction { + transferTx := newNEP17TransferWithAssert(tokenHash, testchain.MultisigScriptHash(), to, amount, needAssert, additionalArgs...) transferTx.SystemFee = 100000000 transferTx.ValidUntilBlock = chain.BlockHeight() + 1 addSigners(neoOwner, transferTx) diff --git a/pkg/core/native/native_nep17.go b/pkg/core/native/native_nep17.go index bbc55bec7..4e2b9deb6 100644 --- a/pkg/core/native/native_nep17.go +++ b/pkg/core/native/native_nep17.go @@ -163,7 +163,9 @@ func (c *nep17TokenNative) emitTransfer(ic *interop.Context, from, to *util.Uint ic.Notifications = append(ic.Notifications, ne) } -func (c *nep17TokenNative) updateAccBalance(ic *interop.Context, acc util.Uint160, amount *big.Int) error { +// updateAccBalance adds specified amount to the acc's balance. If requiredBalance +// is set and amount is 0, then acc's balance is checked against requiredBalance. +func (c *nep17TokenNative) updateAccBalance(ic *interop.Context, acc util.Uint160, amount *big.Int, requiredBalance *big.Int) error { key := makeAccountKey(acc) si := ic.DAO.GetStorageItem(c.ID, key) if si == nil { @@ -171,6 +173,24 @@ func (c *nep17TokenNative) updateAccBalance(ic *interop.Context, acc util.Uint16 return errors.New("insufficient funds") } si = state.StorageItem{} + } else if amount.Sign() == 0 && requiredBalance != nil { + // If amount == 0 then it's either a roundtrip or an empty transfer. In + // case of a roundtrip account's balance may still be less than actual + // transfer's amount, so we need to check it. Other cases are handled by + // `incBalance` method. + balance, err := c.balFromBytes(&si) + if err != nil { + return fmt.Errorf("failed to deserialise balance: %w", err) + } + if balance.Cmp(requiredBalance) < 0 { + // Firstly, need to put it back to storage as it affects dumps. + err = ic.DAO.PutStorageItem(c.ID, key, si) + if err != nil { + return err + } + // Finally, return an error. + return errors.New("insufficient funds") + } } err := c.incBalance(ic, acc, &si, amount) @@ -207,12 +227,12 @@ func (c *nep17TokenNative) TransferInternal(ic *interop.Context, from, to util.U } else { inc = new(big.Int).Neg(inc) } - if err := c.updateAccBalance(ic, from, inc); err != nil { + if err := c.updateAccBalance(ic, from, inc, amount); err != nil { return err } if !isEmpty { - if err := c.updateAccBalance(ic, to, amount); err != nil { + if err := c.updateAccBalance(ic, to, amount, nil); err != nil { return err } } diff --git a/pkg/core/native_gas_test.go b/pkg/core/native_gas_test.go index 8b7e06e9d..dbc358f95 100644 --- a/pkg/core/native_gas_test.go +++ b/pkg/core/native_gas_test.go @@ -5,7 +5,10 @@ import ( "testing" "github.com/nspcc-dev/neo-go/internal/random" + "github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger" + "github.com/nspcc-dev/neo-go/pkg/util" "github.com/nspcc-dev/neo-go/pkg/vm" + "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" "github.com/nspcc-dev/neo-go/pkg/wallet" "github.com/stretchr/testify/require" ) @@ -68,3 +71,48 @@ func TestGAS_Refuel(t *testing.T) { require.Equal(t, vm.FaultState, aer.VMState) }) } + +func TestGAS_Roundtrip(t *testing.T) { + bc := newTestChain(t) + + getUtilityTokenBalance := func(bc *Blockchain, acc util.Uint160) (*big.Int, uint32) { + bs, err := bc.dao.GetNEP17Balances(acc) + if err != nil { + return big.NewInt(0), 0 + } + balance := bs.Trackers[bc.contracts.GAS.ID] + return &balance.Balance, balance.LastUpdatedBlock + } + + initialBalance, _ := getUtilityTokenBalance(bc, neoOwner) + require.NotNil(t, initialBalance) + + t.Run("bad: amount > initial balance", func(t *testing.T) { + tx := transferTokenFromMultisigAccountWithAssert(t, bc, neoOwner, bc.contracts.GAS.Hash, initialBalance.Int64()+1, false) + aer, err := bc.GetAppExecResults(tx.Hash(), trigger.Application) + require.NoError(t, err) + require.Equal(t, vm.HaltState, aer[0].VMState, aer[0].FaultException) // transfer without assert => HALT state + checkResult(t, &aer[0], stackitem.NewBool(false)) + require.Len(t, aer[0].Events, 0) // failed transfer => no events + // check balance and height were not changed + updatedBalance, updatedHeight := getUtilityTokenBalance(bc, neoOwner) + initialBalance.Sub(initialBalance, big.NewInt(tx.SystemFee+tx.NetworkFee)) + require.Equal(t, initialBalance, updatedBalance) + require.Equal(t, bc.BlockHeight(), updatedHeight) + }) + + t.Run("good: amount < initial balance", func(t *testing.T) { + amount := initialBalance.Int64() - 10_00000000 + tx := transferTokenFromMultisigAccountWithAssert(t, bc, neoOwner, bc.contracts.GAS.Hash, amount, false) + aer, err := bc.GetAppExecResults(tx.Hash(), trigger.Application) + require.NoError(t, err) + require.Equal(t, vm.HaltState, aer[0].VMState, aer[0].FaultException) + checkResult(t, &aer[0], stackitem.NewBool(true)) + require.Len(t, aer[0].Events, 1) // roundtrip + // check balance wasn't changed and height was updated + updatedBalance, updatedHeight := getUtilityTokenBalance(bc, neoOwner) + initialBalance.Sub(initialBalance, big.NewInt(tx.SystemFee+tx.NetworkFee)) + require.Equal(t, initialBalance, updatedBalance) + require.Equal(t, bc.BlockHeight(), updatedHeight) + }) +} diff --git a/pkg/core/native_neo_test.go b/pkg/core/native_neo_test.go index f5766d565..87ff08ab4 100644 --- a/pkg/core/native_neo_test.go +++ b/pkg/core/native_neo_test.go @@ -325,3 +325,35 @@ func TestRegisterPrice(t *testing.T) { testGetSet(t, bc, bc.contracts.NEO.Hash, "RegisterPrice", native.DefaultRegisterPrice, 1, math.MaxInt64) } + +func TestNEO_Roundtrip(t *testing.T) { + bc := newTestChain(t) + initialBalance, initialHeight := bc.GetGoverningTokenBalance(neoOwner) + require.NotNil(t, initialBalance) + + t.Run("bad: amount > initial balance", func(t *testing.T) { + tx := transferTokenFromMultisigAccountWithAssert(t, bc, neoOwner, bc.contracts.NEO.Hash, initialBalance.Int64()+1, false) + aer, err := bc.GetAppExecResults(tx.Hash(), trigger.Application) + require.NoError(t, err) + require.Equal(t, vm.HaltState, aer[0].VMState, aer[0].FaultException) // transfer without assert => HALT state + checkResult(t, &aer[0], stackitem.NewBool(false)) + require.Len(t, aer[0].Events, 0) // failed transfer => no events + // check balance and height were not changed + updatedBalance, updatedHeight := bc.GetGoverningTokenBalance(neoOwner) + require.Equal(t, initialBalance, updatedBalance) + require.Equal(t, initialHeight, updatedHeight) + }) + + t.Run("good: amount == initial balance", func(t *testing.T) { + tx := transferTokenFromMultisigAccountWithAssert(t, bc, neoOwner, bc.contracts.NEO.Hash, initialBalance.Int64(), false) + aer, err := bc.GetAppExecResults(tx.Hash(), trigger.Application) + require.NoError(t, err) + require.Equal(t, vm.HaltState, aer[0].VMState, aer[0].FaultException) + checkResult(t, &aer[0], stackitem.NewBool(true)) + require.Len(t, aer[0].Events, 2) // roundtrip + GAS claim + // check balance wasn't changed and height was updated + updatedBalance, updatedHeight := bc.GetGoverningTokenBalance(neoOwner) + require.Equal(t, initialBalance, updatedBalance) + require.Equal(t, bc.BlockHeight(), updatedHeight) + }) +}