Merge pull request #2007 from nspcc-dev/fix-diff-92057

core: do not allow NEP17 roundtrip in case of insufficient funds

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)

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
 		}
 	}

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)
+	})
+}

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)
+	})
+}