Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter validation of EIP-7702 transactions #11885

Merged
merged 14 commits into from
Sep 12, 2024
4 changes: 3 additions & 1 deletion core/types/authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import (

libcommon "github.com/erigontech/erigon-lib/common"
"github.com/erigontech/erigon-lib/common/length"
libcrypto "github.com/erigontech/erigon-lib/crypto"
rlp2 "github.com/erigontech/erigon-lib/rlp"

"github.com/erigontech/erigon/common/u256"
"github.com/erigontech/erigon/crypto"
"github.com/erigontech/erigon/params"
Expand Down Expand Up @@ -75,7 +77,7 @@ func (ath *Authorization) RecoverSigner(data *bytes.Buffer, b []byte) (*libcommo
return nil, fmt.Errorf("invalid v value: %d", ath.V.Uint64())
}

if !crypto.ValidateSignatureValues(sig[64], &ath.R, &ath.S, false) {
if !libcrypto.TransactionSignatureIsValid(sig[64], &ath.R, &ath.S, false /* allowPreEip2s */) {
sudeepdino008 marked this conversation as resolved.
Show resolved Hide resolved
return nil, errors.New("invalid signature")
}

Expand Down
12 changes: 7 additions & 5 deletions core/types/set_code_tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,11 @@ func (tx *SetCodeTransaction) AsMessage(s Signer, baseFee *big.Int, rules *chain
msg.gasPrice.Set(tx.FeeCap)
}

if len(tx.Authorizations) == 0 {
return msg, errors.New("SetCodeTransaction without authorizations is invalid")
}
msg.authorizations = tx.Authorizations

var err error
msg.from, err = tx.Sender(s)
return msg, err
Expand Down Expand Up @@ -225,13 +229,11 @@ func (tx *SetCodeTransaction) DecodeRLP(s *rlp.Stream) error {
if b, err = s.Bytes(); err != nil {
return err
}
if len(b) > 0 && len(b) != 20 {
if len(b) != 20 {
return fmt.Errorf("wrong size for To: %d", len(b))
}
if len(b) > 0 {
tx.To = &libcommon.Address{}
copy((*tx.To)[:], b)
}
tx.To = &libcommon.Address{}
copy((*tx.To)[:], b)
if b, err = s.Uint256Bytes(); err != nil {
return err
}
Expand Down
7 changes: 3 additions & 4 deletions core/types/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,14 @@ import (
"github.com/holiman/uint256"
"github.com/protolambda/ztyp/codec"

"github.com/erigontech/erigon-lib/log/v3"

"github.com/erigontech/erigon-lib/chain"
libcommon "github.com/erigontech/erigon-lib/common"
"github.com/erigontech/erigon-lib/common/fixedgas"
libcrypto "github.com/erigontech/erigon-lib/crypto"
"github.com/erigontech/erigon-lib/log/v3"
types2 "github.com/erigontech/erigon-lib/types"

"github.com/erigontech/erigon/common/math"
"github.com/erigontech/erigon/crypto"
"github.com/erigontech/erigon/rlp"
)

Expand Down Expand Up @@ -284,7 +283,7 @@ func sanityCheckSignature(v *uint256.Int, r *uint256.Int, s *uint256.Int, maybeP
// must already be equal to the recovery id.
plainV = byte(v.Uint64())
}
if !crypto.ValidateSignatureValues(plainV, r, s, false) {
if !libcrypto.TransactionSignatureIsValid(plainV, r, s, true /* allowPreEip2s */) {
return ErrInvalidSig
}

Expand Down
3 changes: 2 additions & 1 deletion core/types/transaction_signing.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (

"github.com/erigontech/erigon-lib/chain"
libcommon "github.com/erigontech/erigon-lib/common"
libcrypto "github.com/erigontech/erigon-lib/crypto"

"github.com/erigontech/erigon/common/u256"
"github.com/erigontech/erigon/crypto"
Expand Down Expand Up @@ -364,7 +365,7 @@ func recoverPlain(context *secp256k1.Context, sighash libcommon.Hash, R, S, Vb *
return libcommon.Address{}, ErrInvalidSig
}
V := byte(Vb.Uint64() - 27)
if !crypto.ValidateSignatureValues(V, R, S, homestead) {
if !libcrypto.TransactionSignatureIsValid(V, R, S, !homestead) {
return libcommon.Address{}, ErrInvalidSig
}
// encode the signature in uncompressed format
Expand Down
3 changes: 2 additions & 1 deletion core/vm/contracts.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (

"github.com/erigontech/erigon-lib/chain"
libcommon "github.com/erigontech/erigon-lib/common"
libcrypto "github.com/erigontech/erigon-lib/crypto"
"github.com/erigontech/erigon-lib/crypto/blake2b"
libkzg "github.com/erigontech/erigon-lib/crypto/kzg"

Expand Down Expand Up @@ -242,7 +243,7 @@ func (c *ecrecover) Run(input []byte) ([]byte, error) {
v := input[63] - 27

// tighter sig s values input homestead only apply to txn sigs
if !allZero(input[32:63]) || !crypto.ValidateSignatureValues(v, r, s, false) {
if !allZero(input[32:63]) || !libcrypto.TransactionSignatureIsValid(v, r, s, true /* allowPreEip2s */) {
return nil, nil
}
// We must make sure not to modify the 'input', so placing the 'v' along with
Expand Down
15 changes: 0 additions & 15 deletions crypto/crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -301,21 +301,6 @@ func GenerateKey() (*ecdsa.PrivateKey, error) {
return ecdsa.GenerateKey(S256(), rand.Reader)
}

// ValidateSignatureValues verifies whether the signature values are valid with
// the given chain rules. The v value is assumed to be either 0 or 1.
func ValidateSignatureValues(v byte, r, s *uint256.Int, homestead bool) bool {
if r.IsZero() || s.IsZero() {
return false
}
// reject upper range of s values (ECDSA malleability)
// see discussion in secp256k1/libsecp256k1/include/secp256k1.h
if homestead && s.Gt(secp256k1halfN) {
return false
}
// Frontier: allow s to be in full N range
return r.Lt(secp256k1N) && s.Lt(secp256k1N) && (v == 0 || v == 1)
}

// DESCRIBED: docs/programmers_guide/guide.md#address---identifier-of-an-account
func PubkeyToAddress(p ecdsa.PublicKey) libcommon.Address {
pubBytes := MarshalPubkey(&p)
Expand Down
48 changes: 0 additions & 48 deletions crypto/crypto_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,12 @@ import (
"reflect"
"testing"

"github.com/holiman/uint256"
"golang.org/x/crypto/sha3"

libcommon "github.com/erigontech/erigon-lib/common"
"github.com/erigontech/erigon-lib/common/hexutil"

"github.com/erigontech/erigon/common"
"github.com/erigontech/erigon/common/u256"
)

var testAddrHex = "970e8128ab834e8eac17ab8e3812f010678cf791"
Expand Down Expand Up @@ -279,52 +277,6 @@ func TestSaveECDSA(t *testing.T) {
}
}

func TestValidateSignatureValues(t *testing.T) {
check := func(expected bool, v byte, r, s *uint256.Int) {
if ValidateSignatureValues(v, r, s, false) != expected {
t.Errorf("mismatch for v: %d r: %d s: %d want: %v", v, r, s, expected)
}
}
minusOne := uint256.NewInt(0).SetAllOne()
one := u256.Num1
zero := u256.Num0
secp256k1nMinus1 := new(uint256.Int).Sub(secp256k1N, u256.Num1)

// correct v,r,s
check(true, 0, one, one)
check(true, 1, one, one)
// incorrect v, correct r,s,
check(false, 2, one, one)
check(false, 3, one, one)

// incorrect v, combinations of incorrect/correct r,s at lower limit
check(false, 2, zero, zero)
check(false, 2, zero, one)
check(false, 2, one, zero)
check(false, 2, one, one)

// correct v for any combination of incorrect r,s
check(false, 0, zero, zero)
check(false, 0, zero, one)
check(false, 0, one, zero)

check(false, 1, zero, zero)
check(false, 1, zero, one)
check(false, 1, one, zero)

// correct sig with max r,s
check(true, 0, secp256k1nMinus1, secp256k1nMinus1)
// correct v, combinations of incorrect r,s at upper limit
check(false, 0, secp256k1N, secp256k1nMinus1)
check(false, 0, secp256k1nMinus1, secp256k1N)
check(false, 0, secp256k1N, secp256k1N)

// current callers ensures r,s cannot be negative, but let's test for that too
// as crypto package could be used stand-alone
check(false, 0, minusOne, one)
check(false, 0, one, minusOne)
}

func checkhash(t *testing.T, name string, f func([]byte) []byte, msg, exp []byte) {
sum := f(msg)
if !bytes.Equal(exp, sum) {
Expand Down
4 changes: 2 additions & 2 deletions erigon-lib/crypto/secp256k1.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (

var (
secp256k1N = new(uint256.Int).SetBytes(hexutility.MustDecodeHex("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141"))
secp256k1halfN = new(uint256.Int).Rsh(secp256k1N, 1)
Secp256k1halfN = new(uint256.Int).Rsh(secp256k1N, 1)
)

// See Appendix F "Signing Transactions" of the Yellow Paper
Expand All @@ -34,7 +34,7 @@ func TransactionSignatureIsValid(v byte, r, s *uint256.Int, allowPreEip2s bool)
}

// See EIP-2: Homestead Hard-fork Changes
if !allowPreEip2s && s.Gt(secp256k1halfN) {
if !allowPreEip2s && s.Gt(Secp256k1halfN) {
return false
}

Expand Down
74 changes: 74 additions & 0 deletions erigon-lib/crypto/secp256k1_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright 2014 The go-ethereum Authors
// (original work)
// Copyright 2024 The Erigon Authors
// (modifications)
// This file is part of Erigon.
//
// Erigon is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Erigon is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with Erigon. If not, see <http://www.gnu.org/licenses/>.

package crypto

import (
"testing"

"github.com/holiman/uint256"

"github.com/erigontech/erigon-lib/common/u256"
)

func TestTransactionSignatureIsValid(t *testing.T) {
check := func(expected bool, v byte, r, s *uint256.Int) {
if TransactionSignatureIsValid(v, r, s, true) != expected {
t.Errorf("mismatch for v: %d r: %d s: %d want: %v", v, r, s, expected)
}
}
minusOne := uint256.NewInt(0).SetAllOne()
one := u256.N1
zero := u256.N0
secp256k1nMinus1 := new(uint256.Int).Sub(secp256k1N, u256.N1)

// correct v,r,s
check(true, 0, one, one)
check(true, 1, one, one)
// incorrect v, correct r,s,
check(false, 2, one, one)
check(false, 3, one, one)

// incorrect v, combinations of incorrect/correct r,s at lower limit
check(false, 2, zero, zero)
check(false, 2, zero, one)
check(false, 2, one, zero)
check(false, 2, one, one)

// correct v for any combination of incorrect r,s
check(false, 0, zero, zero)
check(false, 0, zero, one)
check(false, 0, one, zero)

check(false, 1, zero, zero)
check(false, 1, zero, one)
check(false, 1, one, zero)

// correct sig with max r,s
check(true, 0, secp256k1nMinus1, secp256k1nMinus1)
// correct v, combinations of incorrect r,s at upper limit
check(false, 0, secp256k1N, secp256k1nMinus1)
check(false, 0, secp256k1nMinus1, secp256k1N)
check(false, 0, secp256k1N, secp256k1N)

// current callers ensures r,s cannot be negative, but let's test for that too
// as crypto package could be used stand-alone
check(false, 0, minusOne, one)
check(false, 0, one, minusOne)
}
20 changes: 17 additions & 3 deletions erigon-lib/txpool/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"github.com/erigontech/erigon-lib/common/fixedgas"
"github.com/erigontech/erigon-lib/common/hexutility"
"github.com/erigontech/erigon-lib/common/u256"
"github.com/erigontech/erigon-lib/crypto"
libkzg "github.com/erigontech/erigon-lib/crypto/kzg"
"github.com/erigontech/erigon-lib/gointerfaces"
"github.com/erigontech/erigon-lib/gointerfaces/grpcutil"
Expand Down Expand Up @@ -772,7 +773,8 @@ func (p *TxPool) best(n uint16, txs *types.TxsRlp, tx kv.Tx, onTopOf, availableG
// make sure we have enough gas in the caller to add this transaction.
// not an exact science using intrinsic gas but as close as we could hope for at
// this stage
intrinsicGas, _ := txpoolcfg.CalcIntrinsicGas(uint64(mt.Tx.DataLen), uint64(mt.Tx.DataNonZeroLen), uint64(mt.Tx.AuthorizationLen), nil, mt.Tx.Creation, true, true, isShanghai)
authorizationLen := uint64(len(mt.Tx.Authorizations))
intrinsicGas, _ := txpoolcfg.CalcIntrinsicGas(uint64(mt.Tx.DataLen), uint64(mt.Tx.DataNonZeroLen), authorizationLen, nil, mt.Tx.Creation, true, true, isShanghai)
if intrinsicGas > availableGas {
// we might find another txn with a low enough intrinsic gas to include so carry on
continue
Expand Down Expand Up @@ -852,7 +854,7 @@ func (p *TxPool) validateTx(txn *types.TxSlot, isLocal bool, stateCache kvcache.
return txpoolcfg.TypeNotActivated
}
if txn.Creation {
return txpoolcfg.CreateBlobTxn
return txpoolcfg.InvalidCreateTxn
}
blobCount := uint64(len(txn.BlobHashes))
if blobCount == 0 {
Expand Down Expand Up @@ -896,10 +898,22 @@ func (p *TxPool) validateTx(txn *types.TxSlot, isLocal bool, stateCache kvcache.
}
}

authorizationLen := len(txn.Authorizations)
if txn.Type == types.SetCodeTxType {
if !p.isPrague() {
return txpoolcfg.TypeNotActivated
}
if txn.Creation {
return txpoolcfg.InvalidCreateTxn
}
if authorizationLen == 0 {
return txpoolcfg.NoAuthorizations
}
for i := 0; i < authorizationLen; i++ {
if txn.Authorizations[i].S.Gt(crypto.Secp256k1halfN) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just use TransactionSignatureIsValid here, as it it used in authority recover, and if it fails there, the entire tx fails. So maybe we can check all aspects of the signature
ref: link

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but I'm holding off until ethereum/EIPs#8865 is merged.

return txpoolcfg.InvalidAuthorization
}
}
}

// Drop non-local transactions under our own minimal accepted gas price or tip
Expand All @@ -909,7 +923,7 @@ func (p *TxPool) validateTx(txn *types.TxSlot, isLocal bool, stateCache kvcache.
}
return txpoolcfg.UnderPriced
}
gas, reason := txpoolcfg.CalcIntrinsicGas(uint64(txn.DataLen), uint64(txn.DataNonZeroLen), uint64(txn.AuthorizationLen), nil, txn.Creation, true, true, isShanghai)
gas, reason := txpoolcfg.CalcIntrinsicGas(uint64(txn.DataLen), uint64(txn.DataNonZeroLen), uint64(authorizationLen), nil, txn.Creation, true, true, isShanghai)
if txn.Traced {
p.logger.Info(fmt.Sprintf("TX TRACING: validateTx intrinsic gas idHash=%x gas=%d", txn.IDHash, gas))
}
Expand Down
7 changes: 5 additions & 2 deletions erigon-lib/txpool/txpool_grpc_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,11 @@ func mapDiscardReasonToProto(reason txpoolcfg.DiscardReason) txpool_proto.Import
return txpool_proto.ImportResult_ALREADY_EXISTS
case txpoolcfg.UnderPriced, txpoolcfg.ReplaceUnderpriced, txpoolcfg.FeeTooLow:
return txpool_proto.ImportResult_FEE_TOO_LOW
case txpoolcfg.InvalidSender, txpoolcfg.NegativeValue, txpoolcfg.OversizedData, txpoolcfg.InitCodeTooLarge, txpoolcfg.RLPTooLong, txpoolcfg.CreateBlobTxn, txpoolcfg.NoBlobs, txpoolcfg.TooManyBlobs, txpoolcfg.TypeNotActivated, txpoolcfg.UnequalBlobTxExt, txpoolcfg.BlobHashCheckFail, txpoolcfg.UnmatchedBlobTxExt:
// TODO(eip-4844) TypeNotActivated may be transient (e.g. a blob transaction is submitted 1 sec prior to Cancun activation)
case txpoolcfg.InvalidSender, txpoolcfg.NegativeValue, txpoolcfg.OversizedData, txpoolcfg.InitCodeTooLarge,
txpoolcfg.RLPTooLong, txpoolcfg.InvalidCreateTxn, txpoolcfg.NoBlobs, txpoolcfg.TooManyBlobs,
txpoolcfg.TypeNotActivated, txpoolcfg.UnequalBlobTxExt, txpoolcfg.BlobHashCheckFail,
txpoolcfg.UnmatchedBlobTxExt, txpoolcfg.NoAuthorizations, txpoolcfg.InvalidAuthorization:
// TODO(EIP-7702) TypeNotActivated may be transient (e.g. a set code transaction is submitted 1 sec prior to the Pectra activation)
return txpool_proto.ImportResult_INVALID
default:
return txpool_proto.ImportResult_INTERNAL_ERROR
Expand Down
Loading
Loading