From badae30d5c374eb54de711de428313628115a149 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 18 Dec 2018 12:24:18 +0100 Subject: [PATCH] x/auth: fetch one account after another - Don't read all accounts in one go as signature verification could fail before last signature is checked. - TestAnteHandlerFees now checks whether fees were actually deducted from the account as well as the fee keeper collected them. Thanks: @ValarDragon for pointing this out Closes: #3093 --- PENDING.md | 2 ++ x/auth/ante.go | 61 ++++++++++++++++++++++++--------------------- x/auth/ante_test.go | 7 +++++- 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/PENDING.md b/PENDING.md index 4d87301f0d5b..6906a4fb70d4 100644 --- a/PENDING.md +++ b/PENDING.md @@ -35,6 +35,8 @@ IMPROVEMENTS * Gaia * SDK + * [\#3093](https://github.com/cosmos/cosmos-sdk/issues/3093) Ante handler does no longer read all accounts in one go when processing signatures as signature + verification may fail before last signature is checked. * Tendermint diff --git a/x/auth/ante.go b/x/auth/ante.go index 33fd5ae5e9e9..f05b20c6de66 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -76,12 +76,17 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo") - signerAccs, res := GetSignerAccs(newCtx, am, stdTx.GetSigners()) + // stdSigs contains the sequence number, account number, and signatures. + // When simulating, this would just be a 0-length slice. + signerAddrs := stdTx.GetSigners() + signerAccs := make([]Account, len(signerAddrs)) + isGenesis := ctx.BlockHeight() == 0 + + // fetch first signer, who's going to pay the fees + signerAccs[0], res = GetSignerAcc(newCtx, am, signerAddrs[0]) if !res.IsOK() { return newCtx, res, true } - - // the first signer pays the transaction fees if !stdTx.Fee.Amount.IsZero() { signerAccs[0], res = DeductFees(signerAccs[0], stdTx.Fee) if !res.IsOK() { @@ -91,16 +96,21 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { fck.AddCollectedFees(newCtx, stdTx.Fee.Amount) } - isGenesis := ctx.BlockHeight() == 0 - signBytesList := GetSignBytesList(newCtx.ChainID(), stdTx, signerAccs, isGenesis) - // stdSigs contains the sequence number, account number, and signatures. // When simulating, this would just be a 0-length slice. stdSigs := stdTx.GetSignatures() for i := 0; i < len(stdSigs); i++ { + // skip the fee payer, account is cached and fees were deducted already + if i != 0 { + signerAccs[i], res = GetSignerAcc(newCtx, am, signerAddrs[i]) + if !res.IsOK() { + return newCtx, res, true + } + } // check signature, return account with incremented nonce - signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytesList[i], simulate) + signBytes := GetSignBytes(newCtx.ChainID(), stdTx, signerAccs[i], isGenesis) + signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytes, simulate) if !res.IsOK() { return newCtx, res, true } @@ -116,17 +126,13 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { } } -// GetSignerAccs returns a list of signers for a given list of addresses that -// are expected to sign a transaction. -func GetSignerAccs(ctx sdk.Context, am AccountKeeper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) { - accs = make([]Account, len(addrs)) - for i := 0; i < len(accs); i++ { - accs[i] = am.GetAccount(ctx, addrs[i]) - if accs[i] == nil { - return nil, sdk.ErrUnknownAddress(addrs[i].String()).Result() - } +// GetSignerAcc a signers for a given address that is expected to sign a transaction. +func GetSignerAcc(ctx sdk.Context, am AccountKeeper, addr sdk.AccAddress) (acc Account, res sdk.Result) { + acc = am.GetAccount(ctx, addr) + if acc == nil { + res = sdk.ErrUnknownAddress(addr.String()).Result() + return } - return } @@ -293,18 +299,15 @@ func SetGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context { return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas)) } -// GetSignBytesList returns a slice of bytes to sign over for a given transaction -// and list of accounts. -func GetSignBytesList(chainID string, stdTx StdTx, accs []Account, genesis bool) (signatureBytesList [][]byte) { - signatureBytesList = make([][]byte, len(accs)) - for i := 0; i < len(accs); i++ { - accNum := accs[i].GetAccountNumber() - if genesis { - accNum = 0 - } - signatureBytesList[i] = StdSignBytes(chainID, - accNum, accs[i].GetSequence(), - stdTx.Fee, stdTx.Msgs, stdTx.Memo) +// GetSignBytes returns a slice of bytes to sign over for a given transaction +// and an account. +func GetSignBytes(chainID string, stdTx StdTx, acc Account, genesis bool) (signBytes []byte) { + accNum := acc.GetAccountNumber() + if genesis { + accNum = 0 } + signBytes = StdSignBytes(chainID, + accNum, acc.GetSequence(), + stdTx.Fee, stdTx.Msgs, stdTx.Memo) return } diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index 9bf4ab37fe6c..6d4aaba96016 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -396,12 +396,14 @@ func TestAnteHandlerFees(t *testing.T) { checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInsufficientFunds) require.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(emptyCoins)) + require.True(t, mapper.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(149))) acc1.SetCoins(sdk.Coins{sdk.NewInt64Coin("atom", 150)}) mapper.SetAccount(ctx, acc1) checkValidTx(t, anteHandler, ctx, tx, false) require.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(sdk.Coins{sdk.NewInt64Coin("atom", 150)})) + require.True(t, mapper.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(0))) } // Test logic around memo gas consumption. @@ -646,8 +648,10 @@ func TestProcessPubKey(t *testing.T) { ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, log.NewNopLogger()) // keys _, addr1 := privAndAddr() - priv2, _ := privAndAddr() + priv2, addr2 := privAndAddr() acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc2 := mapper.NewAccountWithAddress(ctx, addr2) + acc2.SetPubKey(priv2.PubKey()) type args struct { acc Account sig StdSignature @@ -660,6 +664,7 @@ func TestProcessPubKey(t *testing.T) { }{ {"no sigs, simulate off", args{acc1, StdSignature{}, false}, true}, {"no sigs, simulate on", args{acc1, StdSignature{}, true}, false}, + {"no sigs, account with pub, simulate on", args{acc2, StdSignature{}, true}, false}, {"pubkey doesn't match addr, simulate off", args{acc1, StdSignature{PubKey: priv2.PubKey()}, false}, true}, {"pubkey doesn't match addr, simulate on", args{acc1, StdSignature{PubKey: priv2.PubKey()}, true}, false}, }