From 7c91964cbb0f7600254b372f66b37bb8e138cf1d Mon Sep 17 00:00:00 2001 From: c r Date: Mon, 29 Nov 2021 17:26:47 -0500 Subject: [PATCH] reorder `transfer` checks so as to ensure sending more money than you have to yourself fails with an error (fixing issue 7596) PR #7637, also adds tests to make sure behavior is correct across versions. --- .circleci/config.yml | 5 ++ chain/vm/runtime.go | 2 +- chain/vm/vm.go | 93 ++++++++++++++++++++++---------- itests/self_sent_txn_test.go | 102 +++++++++++++++++++++++++++++++++++ 4 files changed, 174 insertions(+), 28 deletions(-) create mode 100644 itests/self_sent_txn_test.go diff --git a/.circleci/config.yml b/.circleci/config.yml index 2213d08ad..53611d565 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -895,6 +895,11 @@ workflows: suite: itest-sector_terminate target: "./itests/sector_terminate_test.go" + - test: + name: test-itest-self_sent_txn + suite: itest-self_sent_txn + target: "./itests/self_sent_txn_test.go" + - test: name: test-itest-tape suite: itest-tape diff --git a/chain/vm/runtime.go b/chain/vm/runtime.go index 0dbe98224..548b09028 100644 --- a/chain/vm/runtime.go +++ b/chain/vm/runtime.go @@ -332,7 +332,7 @@ func (rt *Runtime) DeleteActor(beneficiary address.Address) { } // Transfer the executing actor's balance to the beneficiary - if err := rt.vm.transfer(rt.Receiver(), beneficiary, act.Balance); err != nil { + if err := rt.vm.transfer(rt.Receiver(), beneficiary, act.Balance, rt.NetworkVersion()); err != nil { panic(aerrors.Fatalf("failed to transfer balance to beneficiary actor: %s", err)) } } diff --git a/chain/vm/vm.go b/chain/vm/vm.go index 36308fe03..16ad5e2a4 100644 --- a/chain/vm/vm.go +++ b/chain/vm/vm.go @@ -339,7 +339,7 @@ func (vm *VM) send(ctx context.Context, msg *types.Message, parent *Runtime, defer rt.chargeGasSafe(newGasCharge("OnMethodInvocationDone", 0, 0)) if types.BigCmp(msg.Value, types.NewInt(0)) != 0 { - if err := vm.transfer(msg.From, msg.To, msg.Value); err != nil { + if err := vm.transfer(msg.From, msg.To, msg.Value, vm.ntwkVersion(ctx, vm.blockHeight)); err != nil { return nil, aerrors.Wrap(err, "failed to transfer funds") } } @@ -869,32 +869,71 @@ func (vm *VM) incrementNonce(addr address.Address) error { }) } -func (vm *VM) transfer(from, to address.Address, amt types.BigInt) aerrors.ActorError { - if from == to { - return nil - } +func (vm *VM) transfer(from, to address.Address, amt types.BigInt, networkVersion network.Version) aerrors.ActorError { + var f *types.Actor + var fromID, toID address.Address + var err error + // switching the order around so that transactions for more than the balance sent to self fail + if networkVersion >= network.Version15 { + if amt.LessThan(types.NewInt(0)) { + return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt) + } - fromID, err := vm.cstate.LookupID(from) - if err != nil { - return aerrors.Fatalf("transfer failed when resolving sender address: %s", err) - } + fromID, err = vm.cstate.LookupID(from) + if err != nil { + return aerrors.Fatalf("transfer failed when resolving sender address: %s", err) + } - toID, err := vm.cstate.LookupID(to) - if err != nil { - return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err) - } + f, err = vm.cstate.GetActor(fromID) + if err != nil { + return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err) + } - if fromID == toID { - return nil - } + if f.Balance.LessThan(amt) { + return aerrors.Newf(exitcode.SysErrInsufficientFunds, "transfer failed, insufficient balance in sender actor: %v", f.Balance) + } - if amt.LessThan(types.NewInt(0)) { - return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt) - } + if from == to { + log.Infow("sending to same address: noop", "from/to addr", from) + return nil + } - f, err := vm.cstate.GetActor(fromID) - if err != nil { - return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err) + toID, err = vm.cstate.LookupID(to) + if err != nil { + return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err) + } + + if fromID == toID { + log.Infow("sending to same actor ID: noop", "from/to actor", fromID) + return nil + } + } else { + if from == to { + return nil + } + + fromID, err = vm.cstate.LookupID(from) + if err != nil { + return aerrors.Fatalf("transfer failed when resolving sender address: %s", err) + } + + toID, err = vm.cstate.LookupID(to) + if err != nil { + return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err) + } + + if fromID == toID { + return nil + } + + if amt.LessThan(types.NewInt(0)) { + return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt) + } + + f, err = vm.cstate.GetActor(fromID) + if err != nil { + return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err) + } } t, err := vm.cstate.GetActor(toID) @@ -902,17 +941,17 @@ func (vm *VM) transfer(from, to address.Address, amt types.BigInt) aerrors.Actor return aerrors.Fatalf("transfer failed when retrieving receiver actor: %s", err) } - if err := deductFunds(f, amt); err != nil { + if err = deductFunds(f, amt); err != nil { return aerrors.Newf(exitcode.SysErrInsufficientFunds, "transfer failed when deducting funds (%s): %s", types.FIL(amt), err) } depositFunds(t, amt) - if err := vm.cstate.SetActor(fromID, f); err != nil { - return aerrors.Fatalf("transfer failed when setting receiver actor: %s", err) + if err = vm.cstate.SetActor(fromID, f); err != nil { + return aerrors.Fatalf("transfer failed when setting sender actor: %s", err) } - if err := vm.cstate.SetActor(toID, t); err != nil { - return aerrors.Fatalf("transfer failed when setting sender actor: %s", err) + if err = vm.cstate.SetActor(toID, t); err != nil { + return aerrors.Fatalf("transfer failed when setting receiver actor: %s", err) } return nil diff --git a/itests/self_sent_txn_test.go b/itests/self_sent_txn_test.go new file mode 100644 index 000000000..846bcff05 --- /dev/null +++ b/itests/self_sent_txn_test.go @@ -0,0 +1,102 @@ +package itests + +import ( + "context" + "testing" + "time" + + "github.com/filecoin-project/go-state-types/network" + + "github.com/filecoin-project/go-state-types/big" + "github.com/filecoin-project/go-state-types/exitcode" + "github.com/filecoin-project/lotus/api" + "github.com/filecoin-project/lotus/chain/types" + "github.com/filecoin-project/lotus/itests/kit" + "github.com/stretchr/testify/require" +) + + +// these tests check that the versioned code in vm.transfer is functioning correctly across versions! +// we reordered the checks to make sure that a transaction with too much money in it sent to yourself will fail instead of succeeding as a noop +// more info in this PR! https://github.com/filecoin-project/lotus/pull/7637 +func TestSelfSentTxnV15(t *testing.T) { + ctx := context.Background() + + kit.QuietMiningLogs() + + client15, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.GenesisNetworkVersion(network.Version15)) + ens.InterconnectAll().BeginMining(10 * time.Millisecond) + + bal, err := client15.WalletBalance(ctx, client15.DefaultKey.Address) + require.NoError(t, err) + + // send self half of account balance + msgHalfBal := &types.Message{ + From: client15.DefaultKey.Address, + To: client15.DefaultKey.Address, + Value: big.Div(bal, big.NewInt(2)), + } + smHalfBal, err := client15.MpoolPushMessage(ctx, msgHalfBal, nil) + require.NoError(t, err) + mLookup, err := client15.StateWaitMsg(ctx, smHalfBal.Cid(), 3, api.LookbackNoLimit, true) + require.NoError(t, err) + require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode) + + msgOverBal := &types.Message{ + From: client15.DefaultKey.Address, + To: client15.DefaultKey.Address, + Value: big.Mul(big.NewInt(2), bal), + GasLimit: 10000000000, + GasPremium: big.NewInt(10000000000), + GasFeeCap: big.NewInt(100000000000), + Nonce: 1, + } + smOverBal, err := client15.WalletSignMessage(ctx, client15.DefaultKey.Address, msgOverBal) + require.NoError(t, err) + smcid, err := client15.MpoolPush(ctx, smOverBal) + require.NoError(t, err) + mLookup, err = client15.StateWaitMsg(ctx, smcid, 3, api.LookbackNoLimit, true) + require.NoError(t, err) + require.Equal(t, exitcode.SysErrInsufficientFunds, mLookup.Receipt.ExitCode) +} + +func TestSelfSentTxnV14(t *testing.T) { + ctx := context.Background() + + kit.QuietMiningLogs() + + client14, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.GenesisNetworkVersion(network.Version14)) + ens.InterconnectAll().BeginMining(10 * time.Millisecond) + + bal, err := client14.WalletBalance(ctx, client14.DefaultKey.Address) + require.NoError(t, err) + + // send self half of account balance + msgHalfBal := &types.Message{ + From: client14.DefaultKey.Address, + To: client14.DefaultKey.Address, + Value: big.Div(bal, big.NewInt(2)), + } + smHalfBal, err := client14.MpoolPushMessage(ctx, msgHalfBal, nil) + require.NoError(t, err) + mLookup, err := client14.StateWaitMsg(ctx, smHalfBal.Cid(), 3, api.LookbackNoLimit, true) + require.NoError(t, err) + require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode) + + msgOverBal := &types.Message{ + From: client14.DefaultKey.Address, + To: client14.DefaultKey.Address, + Value: big.Mul(big.NewInt(2), bal), + GasLimit: 10000000000, + GasPremium: big.NewInt(10000000000), + GasFeeCap: big.NewInt(100000000000), + Nonce: 1, + } + smOverBal, err := client14.WalletSignMessage(ctx, client14.DefaultKey.Address, msgOverBal) + require.NoError(t, err) + smcid, err := client14.MpoolPush(ctx, smOverBal) + require.NoError(t, err) + mLookup, err = client14.StateWaitMsg(ctx, smcid, 3, api.LookbackNoLimit, true) + require.NoError(t, err) + require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode) +}