-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
fix(auth): audit issues with unordered txs #23392
base: main
Are you sure you want to change the base?
Changes from 5 commits
0603c13
9a7dbff
a54dadf
99f2f31
7f37647
3c075bc
7119934
7d722fd
9b61d0d
2f4c25d
5104784
a4ee4d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -320,18 +320,24 @@ func (svd SigVerificationDecorator) consumeSignatureGas( | |
// verifySig will verify the signature of the provided signer account. | ||
func (svd SigVerificationDecorator) verifySig(ctx context.Context, tx sdk.Tx, acc sdk.AccountI, sig signing.SignatureV2, newlyCreated bool) error { | ||
execMode := svd.ak.GetEnvironment().TransactionService.ExecMode(ctx) | ||
if execMode == transaction.ExecModeCheck { | ||
if sig.Sequence < acc.GetSequence() { | ||
unorderedTx, ok := tx.(sdk.TxWithUnordered) | ||
isUnordered := ok && unorderedTx.GetUnordered() | ||
|
||
// only check sequence if the tx is not unordered | ||
if !isUnordered { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @facundomedica can we test this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a test for it 👌 |
||
if execMode == transaction.ExecModeCheck { | ||
if sig.Sequence < acc.GetSequence() { | ||
return errorsmod.Wrapf( | ||
sdkerrors.ErrWrongSequence, | ||
"account sequence mismatch: expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence, | ||
) | ||
} | ||
} else if sig.Sequence != acc.GetSequence() { | ||
return errorsmod.Wrapf( | ||
sdkerrors.ErrWrongSequence, | ||
"account sequence mismatch, expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence, | ||
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence, | ||
) | ||
} | ||
} else if sig.Sequence != acc.GetSequence() { | ||
return errorsmod.Wrapf( | ||
sdkerrors.ErrWrongSequence, | ||
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence, | ||
) | ||
} | ||
|
||
// we're in simulation mode, or in ReCheckTx, or context is not | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,9 @@ import ( | |
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
"github.com/cosmos/cosmos-sdk/types/tx/signing" | ||
"github.com/cosmos/cosmos-sdk/x/auth/ante/unorderedtx" | ||
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" | ||
) | ||
|
||
// bufPool is a pool of bytes.Buffer objects to reduce memory allocations. | ||
|
@@ -146,8 +148,12 @@ func (d *UnorderedTxDecorator) ValidateTx(ctx context.Context, tx transaction.Tx | |
|
||
// TxIdentifier returns a unique identifier for a transaction that is intended to be unordered. | ||
func TxIdentifier(timeout uint64, tx sdk.Tx) ([32]byte, error) { | ||
feetx := tx.(sdk.FeeTx) | ||
if feetx.GetFee().IsZero() { | ||
sigTx, ok := tx.(authsigning.Tx) | ||
if !ok { | ||
return [32]byte{}, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") | ||
} | ||
|
||
if sigTx.GetFee().IsZero() { | ||
return [32]byte{}, errorsmod.Wrap( | ||
sdkerrors.ErrInvalidRequest, | ||
"unordered transaction must have a fee", | ||
|
@@ -159,6 +165,18 @@ func TxIdentifier(timeout uint64, tx sdk.Tx) ([32]byte, error) { | |
buf.Reset() | ||
defer bufPool.Put(buf) | ||
|
||
// Add signatures to the transaction identifier | ||
signatures, err := sigTx.GetSignaturesV2() | ||
if err != nil { | ||
return [32]byte{}, err | ||
} | ||
|
||
for _, sig := range signatures { | ||
if err := addSignatures(sig.Data, buf); err != nil { | ||
return [32]byte{}, err | ||
} | ||
} | ||
|
||
// Use the buffer | ||
for _, msg := range tx.GetMsgs() { | ||
// loop through the messages and write them to the buffer | ||
|
@@ -189,7 +207,7 @@ func TxIdentifier(timeout uint64, tx sdk.Tx) ([32]byte, error) { | |
} | ||
|
||
// write gas to the buffer | ||
if err := binary.Write(buf, binary.LittleEndian, feetx.GetGas()); err != nil { | ||
if err := binary.Write(buf, binary.LittleEndian, sigTx.GetGas()); err != nil { | ||
return [32]byte{}, errorsmod.Wrap( | ||
sdkerrors.ErrInvalidRequest, | ||
"failed to write unordered to buffer", | ||
|
@@ -201,3 +219,27 @@ func TxIdentifier(timeout uint64, tx sdk.Tx) ([32]byte, error) { | |
// Return the Buffer to the pool | ||
return txHash, nil | ||
} | ||
|
||
func addSignatures(sig signing.SignatureData, buf *bytes.Buffer) error { | ||
switch data := sig.(type) { | ||
case *signing.SingleSignatureData: | ||
if _, err := buf.Write(data.Signature); err != nil { | ||
return errorsmod.Wrap( | ||
sdkerrors.ErrInvalidRequest, | ||
"failed to write single signature to buffer", | ||
) | ||
} | ||
return nil | ||
|
||
case *signing.MultiSignatureData: | ||
for _, sigdata := range data.Signatures { | ||
if err := addSignatures(sigdata, buf); err != nil { | ||
return err | ||
} | ||
} | ||
default: | ||
return fmt.Errorf("unexpected SignatureData %T", data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. personal preference: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I understand this one 🤔 |
||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in the old code before but as you touched it ( ;-) ), it would make sense to have this block moved before
snm.senders
is set in L145. We should not add elements to the object before the error cases are handled