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

[CT-1202] logic to handle unauthorized maker orders when authenticato… #2412

Merged
merged 7 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion protocol/mocks/MemClobKeeper.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions protocol/testutil/memclob/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,3 +528,7 @@ func (f *FakeMemClobKeeper) AddOrderToOrderbookSubaccountUpdatesCheck(
) satypes.UpdateResult {
return satypes.Success
}

func (f *FakeMemClobKeeper) MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error {
return nil
}
58 changes: 58 additions & 0 deletions protocol/x/accountplus/keeper/authenticators.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,69 @@ import (
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
jayy04 marked this conversation as resolved.
Show resolved Hide resolved
gogotypes "github.com/cosmos/gogoproto/types"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
)

// MaybeValidateAuthenticators checks if the transaction has authenticators specified and if so,
// validates them. It returns an error if the authenticators are not valid or removed from state.
func (k Keeper) MaybeValidateAuthenticators(ctx sdk.Context, tx sdk.Tx) error {
// Check if the tx had authenticator specified.
specified, txOptions := lib.HasSelectedAuthenticatorTxExtensionSpecified(tx, k.cdc)
if !specified {
return nil
}

// The tx had authenticators specified.
// First make sure smart account flow is enabled.
if active := k.GetIsSmartAccountActive(ctx); !active {
return types.ErrSmartAccountNotActive
}

// Make sure txn is a SigVerifiableTx and get signers from the tx.
sigVerifiableTx, ok := tx.(authsigning.SigVerifiableTx)
if !ok {
return errors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
}
jayy04 marked this conversation as resolved.
Show resolved Hide resolved

signers, err := sigVerifiableTx.GetSigners()
if err != nil {
return err
}

if len(signers) != 1 {
return errors.Wrap(types.ErrTxnHasMultipleSigners, "only one signer is allowed")
}

account := sdk.AccAddress(signers[0])

// Retrieve the selected authenticators from the extension and make sure they are valid, i.e. they
// are registered and not removed from state.
//
// Note that we only verify the existence of the authenticators here without actually
// runnning them. This is because all current authenticators are stateless and do not read/modify any states.
selectedAuthenticators := txOptions.GetSelectedAuthenticators()
for _, authenticatorId := range selectedAuthenticators {
_, err := k.GetInitializedAuthenticatorForAccount(
ctx,
account,
authenticatorId,
)
if err != nil {
return errors.Wrapf(
err,
"selected authenticator (%s, %d) is not registered or removed from state",
account.String(),
authenticatorId,
)
}
jayy04 marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}
jayy04 marked this conversation as resolved.
Show resolved Hide resolved

// AddAuthenticator adds an authenticator to an account, this function is used to add multiple
// authenticators such as SignatureVerifications and AllOfs
func (k Keeper) AddAuthenticator(
Expand Down
5 changes: 5 additions & 0 deletions protocol/x/accountplus/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,9 @@ var (
5,
"Error initializing authenticator",
)
ErrTxnHasMultipleSigners = errorsmod.Register(
ModuleName,
6,
"The transaction has multiple signers",
)
)
11 changes: 9 additions & 2 deletions protocol/x/clob/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1211,8 +1211,15 @@ func TestPrepareCheckState(t *testing.T) {
case *types.Operation_ShortTermOrderPlacement:
order := operation.GetShortTermOrderPlacement()
tempCtx, writeCache := setupCtx.CacheContext()
tempCtx = tempCtx.WithTxBytes(order.Order.GetOrderHash().ToBytes())
_, _, err := ks.ClobKeeper.PlaceShortTermOrder(

txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(operation.GetShortTermOrderPlacement())
require.NoError(t, err)
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
tempCtx = tempCtx.WithTxBytes(bytes)

_, _, err = ks.ClobKeeper.PlaceShortTermOrder(
Comment on lines +1215 to +1222
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure writeCache() is called to commit state changes

After successfully placing the short-term order with PlaceShortTermOrder, you should call writeCache() to commit the changes from the cached context to the main context. Otherwise, the modifications in tempCtx won't persist, and the order placement might not have the intended effect in the test.

tempCtx,
order,
)
Expand Down
21 changes: 21 additions & 0 deletions protocol/x/clob/keeper/authenticators.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package keeper
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, this PR doesn't need to touch ProcessOperations explicitly, because it's already taken care of by calling AnteHandler on the orders right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct.

again, the assumption here is that the light weight validation is sufficient because all current validations are stateless

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, and ProcessOperations do run the anthenticators explicitly since we cannot trust the proposer


import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

// MaybeValidateAuthenticators checks if the transaction has authenticators specified and if so,
// validates them. It returns an error if the authenticators are not valid or removed from state.
func (k Keeper) MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the logic here is specific to x/clob - should this function be a method in CLOB or Accountplus?

// Decode the tx from the tx bytes.
tx, err := k.txDecoder(txBytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to decode maker orders for all fills (regardless of smart account) - do we think this is okay because there are not many fews?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning to look at frame graphs after this is merged and deployed and optimize only if needed. I feel like this might be okay since like you said, only a very small percentage of the orders get matched.

note that all crossing maker orders get decoded, not just those that generate a fill

if err != nil {
return err
}

// Perform a light-weight validation of the authenticators via the accountplus module.
//
// Note that alternatively we could have been calling the ante handler directly on this transaction,
// but there are some deadlock issues that are non-trivial to resolve.
return k.accountPlusKeeper.MaybeValidateAuthenticators(ctx, tx)
}
1 change: 1 addition & 0 deletions protocol/x/clob/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ type (
rewardsKeeper types.RewardsKeeper
affiliatesKeeper types.AffiliatesKeeper
revshareKeeper types.RevShareKeeper
accountPlusKeeper types.AccountPlusKeeper
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Issues Found: Incomplete Implementation of accountPlusKeeper in NewKeeper

The NewKeeper function signature in protocol/x/clob/keeper/keeper.go has been updated to include the accountPlusKeeper parameter. However, the implementation does not initialize this new field within the Keeper struct. Additionally, multiple calls to NewKeeper across the project are missing the required accountPlusKeeper parameter.

Affected Areas:

  • protocol/x/clob/keeper/keeper.go: NewKeeper implementation missing initialization of accountPlusKeeper.
  • Numerous instances across the project where NewKeeper is called without the accountPlusKeeper parameter, including but not limited to:
    • protocol/x/stats/keeper/keeper.go
    • protocol/x/subaccounts/keeper/keeper.go
    • protocol/testutil/keeper/assets.go
    • protocol/app/app.go

Please address these issues to ensure that the accountPlusKeeper is properly integrated and initialized throughout the codebase.

🔗 Analysis chain

LGTM. Verify NewKeeper implementation and usage.

The addition of the accountPlusKeeper parameter to the NewKeeper function signature is consistent with the struct change. However, we need to ensure that the implementation properly initializes this new field.

Please verify the following:

  1. The NewKeeper function implementation:

  2. All calls to NewKeeper have been updated:


Action Required: Update All NewKeeper Calls to Include accountPlusKeeper

The NewKeeper function signature has been updated to include the accountPlusKeeper parameter. However, the implementation in protocol/x/clob/keeper/keeper.go does not initialize this new field. Additionally, ensure that all calls to NewKeeper across the project are updated to include the accountPlusKeeper parameter to maintain consistency and prevent potential issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Show the NewKeeper function implementation
rg --type go -A 30 'func NewKeeper\(' protocol/x/clob/keeper/keeper.go

Length of output: 1216


Script:

#!/bin/bash
# Find all calls to NewKeeper in the project
rg --type go 'NewKeeper\(' protocol

Length of output: 5220


Script:

#!/bin/bash
# Identify NewKeeper calls missing the 'accountPlusKeeper' parameter
rg --type go 'NewKeeper\(' protocol | grep -v 'accountPlusKeeper'

Length of output: 5248


indexerEventManager indexer_manager.IndexerEventManager
streamingManager streamingtypes.FullNodeStreamingManager
Expand Down
22 changes: 20 additions & 2 deletions protocol/x/clob/keeper/liquidations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,16 @@ func TestPlacePerpetualLiquidation(t *testing.T) {

// Create all existing orders.
for _, order := range tc.existingOrders {
_, _, err := ks.ClobKeeper.PlaceShortTermOrder(ctx, &types.MsgPlaceOrder{Order: order})
msg := &types.MsgPlaceOrder{Order: order}

txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msg)
require.NoError(t, err)
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
ctx = ctx.WithTxBytes(bytes)

_, _, err = ks.ClobKeeper.PlaceShortTermOrder(ctx, msg)
Comment on lines +355 to +364
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor duplicated code for setting TxBytes into a helper function

The code for creating a transaction builder, setting messages, encoding the transaction, and updating ctx.TxBytes is duplicated. Consider refactoring this code into a helper function to improve maintainability and reduce code duplication.

You can create a helper function like this:

func setContextTxBytes(ctx sdk.Context, msg sdk.Msg) (sdk.Context, error) {
  txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
  err := txBuilder.SetMsgs(msg)
  if err != nil {
    return ctx, err
  }
  bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
  if err != nil {
    return ctx, err
  }
  return ctx.WithTxBytes(bytes), nil
}

Then, replace the duplicated code with:

-    txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
-    err := txBuilder.SetMsgs(msg)
-    require.NoError(t, err)
-    bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
-    require.NoError(t, err)
-    ctx = ctx.WithTxBytes(bytes)
+    ctx, err = setContextTxBytes(ctx, msg)
+    require.NoError(t, err)

require.NoError(t, err)
}

Expand Down Expand Up @@ -1306,7 +1315,16 @@ func TestPlacePerpetualLiquidation_PreexistingLiquidation(t *testing.T) {
require.NoError(t, err)
} else {
order := matchableOrder.MustGetOrder()
_, _, err := ks.ClobKeeper.PlaceShortTermOrder(ctx, &types.MsgPlaceOrder{Order: order.MustGetOrder()})
msg := &types.MsgPlaceOrder{Order: order}

txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msg)
require.NoError(t, err)
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
ctx = ctx.WithTxBytes(bytes)

_, _, err = ks.ClobKeeper.PlaceShortTermOrder(ctx, msg)
require.NoError(t, err)
}
}
Expand Down
19 changes: 18 additions & 1 deletion protocol/x/clob/keeper/orders_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,16 @@ func TestPlaceShortTermOrder(t *testing.T) {

// Create all existing orders.
for _, order := range tc.existingOrders {
_, _, err := ks.ClobKeeper.PlaceShortTermOrder(ctx, &types.MsgPlaceOrder{Order: order})
msg := &types.MsgPlaceOrder{Order: order}

txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msg)
require.NoError(t, err)
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
ctx = ctx.WithTxBytes(bytes)

Comment on lines +626 to +634
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor duplicated code into a helper function

The code in lines 626-634 and 644-651 is duplicated. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this repeated code into a helper function.

Apply this diff to refactor the code:

-    msg := &types.MsgPlaceOrder{Order: order}
-
-    txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
-    err := txBuilder.SetMsgs(msg)
-    require.NoError(t, err)
-    bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
-    require.NoError(t, err)
-    ctx = ctx.WithTxBytes(bytes)
+    ctx, err = prepareTxWithMsg(ctx, &types.MsgPlaceOrder{Order: order})
+    require.NoError(t, err)
-    msg := &types.MsgPlaceOrder{Order: tc.order}
-
-    txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
-    err = txBuilder.SetMsgs(msg)
-    require.NoError(t, err)
-    bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
-    require.NoError(t, err)
-    ctx = ctx.WithTxBytes(bytes)
+    ctx, err = prepareTxWithMsg(ctx, &types.MsgPlaceOrder{Order: tc.order})
+    require.NoError(t, err)

Define the helper function outside the selected line ranges:

func prepareTxWithMsg(ctx sdk.Context, msg sdk.Msg) (sdk.Context, error) {
    txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
    err := txBuilder.SetMsgs(msg)
    if err != nil {
        return ctx, err
    }
    bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
    if err != nil {
        return ctx, err
    }
    return ctx.WithTxBytes(bytes), nil
}

Also applies to: 644-651

_, _, err = ks.ClobKeeper.PlaceShortTermOrder(ctx, msg)
require.NoError(t, err)
}

Expand All @@ -632,6 +641,14 @@ func TestPlaceShortTermOrder(t *testing.T) {
ctx.MultiStore().SetTracer(traceDecoder)

msg := &types.MsgPlaceOrder{Order: tc.order}

txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err = txBuilder.SetMsgs(msg)
require.NoError(t, err)
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
ctx = ctx.WithTxBytes(bytes)

orderSizeOptimisticallyFilledFromMatching,
orderStatus,
err := ks.ClobKeeper.PlaceShortTermOrder(ctx, msg)
Expand Down
18 changes: 18 additions & 0 deletions protocol/x/clob/memclob/memclob.go
Original file line number Diff line number Diff line change
Expand Up @@ -1624,6 +1624,24 @@ func (m *MemClobPriceTimePriority) mustPerformTakerOrderMatching(
continue
}

// Perform a lightweight check on maker orders that use the new smart account authentication flow.
// Note that this only applies to short term orders since short term orders go through the ante
// handlers one more time during `DeliverTx`.
if makerOrder.Order.IsShortTermOrder() {
txBytes := m.operationsToPropose.MustGetShortTermOrderTxBytes(makerOrder.Order)
err := m.clobKeeper.MaybeValidateAuthenticators(ctx, txBytes)
if err != nil {
makerOrdersToRemove = append(
makerOrdersToRemove,
OrderWithRemovalReason{
Order: makerOrder.Order,
RemovalReason: types.OrderRemoval_REMOVAL_REASON_PERMISSIONED_KEY_EXPIRED,
},
)
continue
}
}

jayy04 marked this conversation as resolved.
Show resolved Hide resolved
makerRemainingSize, makerHasRemainingSize := m.GetOrderRemainingAmount(ctx, makerOrder.Order)
if !makerHasRemainingSize {
panic(fmt.Sprintf("mustPerformTakerOrderMatching: maker order has no remaining amount %v", makerOrder.Order))
Expand Down
4 changes: 4 additions & 0 deletions protocol/x/clob/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,7 @@ type RevShareKeeper interface {
type AffiliatesKeeper interface {
GetAffiliateWhitelistMap(ctx sdk.Context) (map[string]uint32, error)
}

type AccountPlusKeeper interface {
MaybeValidateAuthenticators(ctx sdk.Context, tx sdk.Tx) error
}
1 change: 1 addition & 0 deletions protocol/x/clob/types/mem_clob_keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,5 @@ type MemClobKeeper interface {
subaccountId satypes.SubaccountId,
order PendingOpenOrder,
) satypes.UpdateResult
MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing MaybeValidateAuthenticators Implementation in MemClobKeeper Interface

The MaybeValidateAuthenticators method has been added to the MemClobKeeper interface, but it appears that many implementations do not include this method. Please ensure that all concrete types implementing MemClobKeeper provide an implementation for MaybeValidateAuthenticators to maintain interface compliance.

  • protocol/streaming/full_node_streaming_manager.go
  • protocol/streaming/noop_streaming_manager.go
  • protocol/streaming/ws/websocket_message_sender.go
  • protocol/streaming/ws/websocket_server.go
  • ... (additional files as identified)
🔗 Analysis chain

New method added to MemClobKeeper interface

The addition of MaybeValidateAuthenticators method aligns with the PR objective of handling unauthorized maker orders. However, there are a few points to consider:

  1. The method name uses "Maybe", suggesting conditional execution. Consider documenting when this validation occurs.
  2. The method operates on raw transaction bytes. Ensure that this level of access is necessary and doesn't bypass any security measures.
  3. As this is an interface method, all implementations of MemClobKeeper will need to be updated to include this method.

To ensure all implementations are updated, run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all implementations of MemClobKeeper and check for MaybeValidateAuthenticators method

# Find all Go files that might implement MemClobKeeper
implementers=$(fd -e go | xargs grep -l "type.*struct.*{")

# Check each potential implementer for the new method
for file in $implementers; do
  if grep -q "func.*MaybeValidateAuthenticators" "$file"; then
    echo "Implementation found in $file"
  else
    echo "Potential missing implementation in $file"
  fi
done

Length of output: 148215

}
24 changes: 24 additions & 0 deletions protocol/x/clob/types/operations_to_propose.go
Original file line number Diff line number Diff line change
Expand Up @@ -475,3 +475,27 @@ func (o *OperationsToPropose) GetOperationsToPropose() []OperationRaw {

return operationRaws
}

// MustGetShortTermOrderTxBytes returns the `ShortTermOrderHashToTxBytes` for a short term order.
// This function will panic for any of the following:
// - the order is not a short term order.
// - the order hash is not present in `ShortTermOrderHashToTxBytes`
func (o *OperationsToPropose) MustGetShortTermOrderTxBytes(
order Order,
) (txBytes []byte) {
order.OrderId.MustBeShortTermOrder()

orderHash := order.GetOrderHash()
bytes, exists := o.ShortTermOrderHashToTxBytes[orderHash]
if !exists {
panic(
fmt.Sprintf(
"MustGetShortTermOrderTxBytes: Order (%s) does not exist in "+
"`ShortTermOrderHashToTxBytes`.",
order.GetOrderTextString(),
),
)
}

return bytes
}
Loading