diff --git a/PENDING.md b/PENDING.md index af4a1193b040..74395be2d5bc 100644 --- a/PENDING.md +++ b/PENDING.md @@ -13,6 +13,7 @@ BREAKING CHANGES * SDK * [\#2752](https://github.com/cosmos/cosmos-sdk/pull/2752) Don't hardcode bondable denom. + * [\#2019](https://github.com/cosmos/cosmos-sdk/issues/2019) Cap total number of signatures. Current per-transaction limit is 7, and if that is exceeded transaction is rejected. * Tendermint diff --git a/docs/reference/baseapp.md b/docs/reference/baseapp.md index 3eaf1e2f7bf4..1828021c2b57 100644 --- a/docs/reference/baseapp.md +++ b/docs/reference/baseapp.md @@ -2,8 +2,7 @@ `baseApp` requires stores to be mounted via capabilities keys - handlers can only access stores they're given the key to. The `baseApp` ensures all stores are properly loaded, cached, and committed. One mounted store is considered the "main" - it holds the latest block header, from which we can find and load the most recent state. -`baseApp` distinguishes between two handler types - the `AnteHandler` and the `MsgHandler`. The former is a global validity check (checking nonces, sigs and sufficient balances to pay fees, -e.g. things that apply to all transaction from all modules), the later is the full state transition function. +`baseApp` distinguishes between two handler types: `AnteHandler` and `MsgHandler`. Whilst the former is a global validity check that applies to all transactions from all modules, i.e. it checks nonces and whether balances are sufficient to pay fees, validates signatures and ensures that transactions don't carry too many signatures, the latter is the full state transition function. During CheckTx the state transition function is only applied to the checkTxState and should return before any expensive state transitions are run (this is up to each developer). It also needs to return the estimated gas cost. diff --git a/types/errors.go b/types/errors.go index e05800b539dd..965a97c872f4 100644 --- a/types/errors.go +++ b/types/errors.go @@ -57,6 +57,7 @@ const ( CodeOutOfGas CodeType = 12 CodeMemoTooLarge CodeType = 13 CodeInsufficientFee CodeType = 14 + CodeTooManySignatures CodeType = 15 // CodespaceRoot is a codespace for error codes in this file only. // Notice that 0 is an "unset" codespace, which can be overridden with @@ -103,6 +104,8 @@ func CodeToDefaultMsg(code CodeType) string { return "memo too large" case CodeInsufficientFee: return "insufficient fee" + case CodeTooManySignatures: + return "maximum numer of signatures exceeded" default: return unknownCodeMsg(code) } @@ -155,6 +158,9 @@ func ErrMemoTooLarge(msg string) Error { func ErrInsufficientFee(msg string) Error { return newErrorWithRootCodespace(CodeInsufficientFee, msg) } +func ErrTooManySignatures(msg string) Error { + return newErrorWithRootCodespace(CodeTooManySignatures, msg) +} //---------------------------------------- // Error & sdkError diff --git a/x/auth/ante.go b/x/auth/ante.go index e88a20a43499..8539be06a42f 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -4,9 +4,11 @@ import ( "bytes" "encoding/hex" "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/crypto/multisig" "github.com/tendermint/tendermint/crypto/secp256k1" ) @@ -17,6 +19,8 @@ const ( maxMemoCharacters = 100 // how much gas = 1 atom gasPerUnitCost = 1000 + // max total number of sigs per tx + txSigLimit = 7 ) // NewAnteHandler returns an AnteHandler that checks @@ -74,6 +78,16 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { stdSigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice. signerAddrs := stdTx.GetSigners() + sigCount := 0 + for i := 0; i < len(stdSigs); i++ { + sigCount += countSubKeys(stdSigs[i].PubKey) + if sigCount > txSigLimit { + return newCtx, sdk.ErrTooManySignatures(fmt.Sprintf( + "signatures: %d, limit: %d", sigCount, txSigLimit), + ).Result(), true + } + } + // create the list of all sign bytes signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, stdSigs) signerAccs, res := getSignerAccs(newCtx, am, signerAddrs) @@ -308,3 +322,15 @@ func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (sign } return } + +func countSubKeys(pub crypto.PubKey) int { + v, ok := pub.(*multisig.PubKeyMultisigThreshold) + if !ok { + return 1 + } + nkeys := 0 + for _, subkey := range v.PubKeys { + nkeys += countSubKeys(subkey) + } + return nkeys +} diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index d29b0bf50de4..98725a74bd31 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -10,6 +10,7 @@ import ( abci "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/crypto/multisig" "github.com/tendermint/tendermint/crypto/secp256k1" "github.com/tendermint/tendermint/libs/log" ) @@ -714,3 +715,77 @@ func TestAdjustFeesByGas(t *testing.T) { }) } } + +func TestCountSubkeys(t *testing.T) { + genPubKeys := func(n int) []crypto.PubKey { + var ret []crypto.PubKey + for i := 0; i < n; i++ { + ret = append(ret, secp256k1.GenPrivKey().PubKey()) + } + return ret + } + genMultiKey := func(n, k int, keysGen func(n int) []crypto.PubKey) crypto.PubKey { + return multisig.NewPubKeyMultisigThreshold(k, keysGen(n)) + } + type args struct { + pub crypto.PubKey + } + mkey := genMultiKey(5, 4, genPubKeys) + mkeyType := mkey.(*multisig.PubKeyMultisigThreshold) + mkeyType.PubKeys = append(mkeyType.PubKeys, genMultiKey(6, 5, genPubKeys)) + tests := []struct { + name string + args args + want int + }{ + {"single key", args{secp256k1.GenPrivKey().PubKey()}, 1}, + {"multi sig key", args{genMultiKey(5, 4, genPubKeys)}, 5}, + {"multi multi sig", args{mkey}, 11}, + } + for _, tt := range tests { + t.Run(tt.name, func(T *testing.T) { + require.Equal(t, tt.want, countSubKeys(tt.args.pub)) + }) + } +} + +func TestAnteHandlerSigLimitExceeded(t *testing.T) { + // setup + ms, capKey, capKey2 := setupMultiStore() + cdc := codec.New() + RegisterBaseAccount(cdc) + mapper := NewAccountKeeper(cdc, capKey, ProtoBaseAccount) + feeCollector := NewFeeCollectionKeeper(cdc, capKey2) + anteHandler := NewAnteHandler(mapper, feeCollector) + ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, log.NewNopLogger()) + ctx = ctx.WithBlockHeight(1) + + // keys and addresses + priv1, addr1 := privAndAddr() + priv2, addr2 := privAndAddr() + priv3, addr3 := privAndAddr() + priv4, addr4 := privAndAddr() + priv5, addr5 := privAndAddr() + priv6, addr6 := privAndAddr() + priv7, addr7 := privAndAddr() + priv8, addr8 := privAndAddr() + + // set the accounts + acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(newCoins()) + mapper.SetAccount(ctx, acc1) + acc2 := mapper.NewAccountWithAddress(ctx, addr2) + acc2.SetCoins(newCoins()) + mapper.SetAccount(ctx, acc2) + + var tx sdk.Tx + msg := newTestMsg(addr1, addr2, addr3, addr4, addr5, addr6, addr7, addr8) + msgs := []sdk.Msg{msg} + fee := newStdFee() + + // test rejection logic + privs, accnums, seqs := []crypto.PrivKey{priv1, priv2, priv3, priv4, priv5, priv6, priv7, priv8}, + []int64{0, 0, 0, 0, 0, 0, 0, 0}, []int64{0, 0, 0, 0, 0, 0, 0, 0} + tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee) + checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeTooManySignatures) +}