From a2eacde0fcef8c480102a49058d001a538a6b1e4 Mon Sep 17 00:00:00 2001 From: jayy04 <103467857+jayy04@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:26:10 -0400 Subject: [PATCH] =?UTF-8?q?[CT-1202]=20logic=20to=20handle=20unauthorized?= =?UTF-8?q?=20maker=20orders=20when=20authenticato=E2=80=A6=20(#2412)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- protocol/app/app.go | 1 + protocol/mocks/MemClobKeeper.go | 20 ++++++- protocol/testutil/keeper/accountplus.go | 8 +-- protocol/testutil/keeper/clob.go | 13 ++++- protocol/testutil/keeper/listing.go | 9 ++- protocol/testutil/memclob/keeper.go | 4 ++ .../x/accountplus/keeper/authenticators.go | 58 +++++++++++++++++++ protocol/x/accountplus/types/errors.go | 5 ++ protocol/x/clob/abci_test.go | 11 +++- protocol/x/clob/keeper/authenticators.go | 21 +++++++ protocol/x/clob/keeper/keeper.go | 3 + protocol/x/clob/keeper/liquidations_test.go | 22 ++++++- protocol/x/clob/keeper/orders_test.go | 19 +++++- protocol/x/clob/memclob/memclob.go | 18 ++++++ protocol/x/clob/types/expected_keepers.go | 4 ++ protocol/x/clob/types/mem_clob_keeper.go | 1 + .../x/clob/types/operations_to_propose.go | 24 ++++++++ 17 files changed, 229 insertions(+), 12 deletions(-) create mode 100644 protocol/x/clob/keeper/authenticators.go diff --git a/protocol/app/app.go b/protocol/app/app.go index 08b46bc042..9796322d21 100644 --- a/protocol/app/app.go +++ b/protocol/app/app.go @@ -1139,6 +1139,7 @@ func New( app.StatsKeeper, app.RewardsKeeper, app.AffiliatesKeeper, + app.AccountPlusKeeper, app.IndexerEventManager, app.FullNodeStreamingManager, txConfig.TxDecoder(), diff --git a/protocol/mocks/MemClobKeeper.go b/protocol/mocks/MemClobKeeper.go index 5d8d68d42f..032e175d12 100644 --- a/protocol/mocks/MemClobKeeper.go +++ b/protocol/mocks/MemClobKeeper.go @@ -288,6 +288,24 @@ func (_m *MemClobKeeper) Logger(ctx types.Context) log.Logger { return r0 } +// MaybeValidateAuthenticators provides a mock function with given fields: ctx, txBytes +func (_m *MemClobKeeper) MaybeValidateAuthenticators(ctx types.Context, txBytes []byte) error { + ret := _m.Called(ctx, txBytes) + + if len(ret) == 0 { + panic("no return value specified for MaybeValidateAuthenticators") + } + + var r0 error + if rf, ok := ret.Get(0).(func(types.Context, []byte) error); ok { + r0 = rf(ctx, txBytes) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // OffsetSubaccountPerpetualPosition provides a mock function with given fields: ctx, liquidatedSubaccountId, perpetualId, deltaQuantumsTotal, isFinalSettlement func (_m *MemClobKeeper) OffsetSubaccountPerpetualPosition(ctx types.Context, liquidatedSubaccountId subaccountstypes.SubaccountId, perpetualId uint32, deltaQuantumsTotal *big.Int, isFinalSettlement bool) ([]clobtypes.MatchPerpetualDeleveraging_Fill, *big.Int) { ret := _m.Called(ctx, liquidatedSubaccountId, perpetualId, deltaQuantumsTotal, isFinalSettlement) @@ -415,7 +433,7 @@ func (_m *MemClobKeeper) ReplayPlaceOrder(ctx types.Context, msg *clobtypes.MsgP return r0, r1, r2, r3 } -// SendOrderbookFillUpdate provides a mock function with given fields: ctx, orderbookFills +// SendOrderbookFillUpdate provides a mock function with given fields: ctx, orderbookFill func (_m *MemClobKeeper) SendOrderbookFillUpdate(ctx types.Context, orderbookFill clobtypes.StreamOrderbookFill) { _m.Called(ctx, orderbookFill) } diff --git a/protocol/testutil/keeper/accountplus.go b/protocol/testutil/keeper/accountplus.go index d518aace3f..8b90b3be0b 100644 --- a/protocol/testutil/keeper/accountplus.go +++ b/protocol/testutil/keeper/accountplus.go @@ -7,7 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/dydxprotocol/v4-chain/protocol/lib" - "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types" + "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types" storetypes "cosmossdk.io/store/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -16,7 +16,7 @@ import ( keeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper" ) -func TimestampNonceKeepers(t testing.TB) ( +func AccountPlusKeepers(t testing.TB) ( ctx sdk.Context, keeper *keeper.Keeper, storeKey storetypes.StoreKey, @@ -32,7 +32,7 @@ func TimestampNonceKeepers(t testing.TB) ( ) []GenesisInitializer { // Define necessary keepers here for unit tests keeper, storeKey, mockTimeProvider = - createTimestampNonceKeeper(stateStore, db, cdc) + createAccountPlusKeeper(stateStore, db, cdc) return []GenesisInitializer{keeper} }, @@ -41,7 +41,7 @@ func TimestampNonceKeepers(t testing.TB) ( return ctx, keeper, storeKey, mockTimeProvider } -func createTimestampNonceKeeper( +func createAccountPlusKeeper( stateStore storetypes.CommitMultiStore, db *dbm.MemDB, cdc *codec.ProtoCodec, diff --git a/protocol/testutil/keeper/clob.go b/protocol/testutil/keeper/clob.go index eddbabd9bf..0e1b1ba1e9 100644 --- a/protocol/testutil/keeper/clob.go +++ b/protocol/testutil/keeper/clob.go @@ -17,6 +17,7 @@ import ( streaming "github.com/dydxprotocol/v4-chain/protocol/streaming" clobtest "github.com/dydxprotocol/v4-chain/protocol/testutil/clob" "github.com/dydxprotocol/v4-chain/protocol/testutil/constants" + accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper" affiliateskeeper "github.com/dydxprotocol/v4-chain/protocol/x/affiliates/keeper" asskeeper "github.com/dydxprotocol/v4-chain/protocol/x/assets/keeper" blocktimekeeper "github.com/dydxprotocol/v4-chain/protocol/x/blocktime/keeper" @@ -51,6 +52,7 @@ type ClobKeepersTestContext struct { SubaccountsKeeper *subkeeper.Keeper AffiliatesKeeper *affiliateskeeper.Keeper VaultKeeper *vaultkeeper.Keeper + AccountPlusKeeper *accountpluskeeper.Keeper StoreKey storetypes.StoreKey MemKey storetypes.StoreKey Cdc *codec.ProtoCodec @@ -91,7 +93,13 @@ func NewClobKeepersTestContextWithUninitializedMemStore( stateStore, db, cdc, - registry) + registry, + ) + ks.AccountPlusKeeper, _, _ = createAccountPlusKeeper( + stateStore, + db, + cdc, + ) stakingKeeper, _ := createStakingKeeper( stateStore, @@ -191,6 +199,7 @@ func NewClobKeepersTestContextWithUninitializedMemStore( ks.AffiliatesKeeper, ks.SubaccountsKeeper, revShareKeeper, + ks.AccountPlusKeeper, indexerEventManager, indexerEventsTransientStoreKey, ) @@ -231,6 +240,7 @@ func createClobKeeper( affiliatesKeeper types.AffiliatesKeeper, saKeeper *subkeeper.Keeper, revShareKeeper types.RevShareKeeper, + accountplusKeeper types.AccountPlusKeeper, indexerEventManager indexer_manager.IndexerEventManager, indexerEventsTransientStoreKey storetypes.StoreKey, ) (*keeper.Keeper, storetypes.StoreKey, storetypes.StoreKey) { @@ -262,6 +272,7 @@ func createClobKeeper( statsKeeper, rewardsKeeper, affiliatesKeeper, + accountplusKeeper, indexerEventManager, streaming.NewNoopGrpcStreamingManager(), constants.TestEncodingCfg.TxConfig.TxDecoder(), diff --git a/protocol/testutil/keeper/listing.go b/protocol/testutil/keeper/listing.go index 824eb50ef5..49f181ecbb 100644 --- a/protocol/testutil/keeper/listing.go +++ b/protocol/testutil/keeper/listing.go @@ -56,7 +56,13 @@ func ListingKeepers( stateStore, db, cdc, - registry) + registry, + ) + accountPlusKeeper, _, _ := createAccountPlusKeeper( + stateStore, + db, + cdc, + ) bankKeeper, _ = createBankKeeper(stateStore, db, cdc, accountsKeeper) stakingKeeper, _ := createStakingKeeper( stateStore, @@ -154,6 +160,7 @@ func ListingKeepers( affiliatesKeeper, subaccountsKeeper, revShareKeeper, + accountPlusKeeper, indexerEventManager, transientStoreKey, ) diff --git a/protocol/testutil/memclob/keeper.go b/protocol/testutil/memclob/keeper.go index 376f6fb30a..3b4282e42e 100644 --- a/protocol/testutil/memclob/keeper.go +++ b/protocol/testutil/memclob/keeper.go @@ -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 +} diff --git a/protocol/x/accountplus/keeper/authenticators.go b/protocol/x/accountplus/keeper/authenticators.go index fc2ac0c013..71d8b9aede 100644 --- a/protocol/x/accountplus/keeper/authenticators.go +++ b/protocol/x/accountplus/keeper/authenticators.go @@ -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" 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") + } + + 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, + ) + } + } + return nil +} + // AddAuthenticator adds an authenticator to an account, this function is used to add multiple // authenticators such as SignatureVerifications and AllOfs func (k Keeper) AddAuthenticator( diff --git a/protocol/x/accountplus/types/errors.go b/protocol/x/accountplus/types/errors.go index 861e5925e0..9307876c66 100644 --- a/protocol/x/accountplus/types/errors.go +++ b/protocol/x/accountplus/types/errors.go @@ -28,4 +28,9 @@ var ( 5, "Error initializing authenticator", ) + ErrTxnHasMultipleSigners = errorsmod.Register( + ModuleName, + 6, + "The transaction has multiple signers", + ) ) diff --git a/protocol/x/clob/abci_test.go b/protocol/x/clob/abci_test.go index 661b8bd4e0..992eb84f37 100644 --- a/protocol/x/clob/abci_test.go +++ b/protocol/x/clob/abci_test.go @@ -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( tempCtx, order, ) diff --git a/protocol/x/clob/keeper/authenticators.go b/protocol/x/clob/keeper/authenticators.go new file mode 100644 index 0000000000..0c78f6478d --- /dev/null +++ b/protocol/x/clob/keeper/authenticators.go @@ -0,0 +1,21 @@ +package keeper + +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 { + // Decode the tx from the tx bytes. + tx, err := k.txDecoder(txBytes) + 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) +} diff --git a/protocol/x/clob/keeper/keeper.go b/protocol/x/clob/keeper/keeper.go index f49eb61271..dbdfdbc0c2 100644 --- a/protocol/x/clob/keeper/keeper.go +++ b/protocol/x/clob/keeper/keeper.go @@ -44,6 +44,7 @@ type ( rewardsKeeper types.RewardsKeeper affiliatesKeeper types.AffiliatesKeeper revshareKeeper types.RevShareKeeper + accountPlusKeeper types.AccountPlusKeeper indexerEventManager indexer_manager.IndexerEventManager streamingManager streamingtypes.FullNodeStreamingManager @@ -88,6 +89,7 @@ func NewKeeper( statsKeeper types.StatsKeeper, rewardsKeeper types.RewardsKeeper, affiliatesKeeper types.AffiliatesKeeper, + accountPlusKeeper types.AccountPlusKeeper, indexerEventManager indexer_manager.IndexerEventManager, streamingManager streamingtypes.FullNodeStreamingManager, txDecoder sdk.TxDecoder, @@ -114,6 +116,7 @@ func NewKeeper( statsKeeper: statsKeeper, rewardsKeeper: rewardsKeeper, affiliatesKeeper: affiliatesKeeper, + accountPlusKeeper: accountPlusKeeper, indexerEventManager: indexerEventManager, streamingManager: streamingManager, memStoreInitialized: &atomic.Bool{}, // False by default. diff --git a/protocol/x/clob/keeper/liquidations_test.go b/protocol/x/clob/keeper/liquidations_test.go index 89809421b6..cd8ffe61fe 100644 --- a/protocol/x/clob/keeper/liquidations_test.go +++ b/protocol/x/clob/keeper/liquidations_test.go @@ -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) require.NoError(t, err) } @@ -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) } } diff --git a/protocol/x/clob/keeper/orders_test.go b/protocol/x/clob/keeper/orders_test.go index b7138d71ab..e8a68b6828 100644 --- a/protocol/x/clob/keeper/orders_test.go +++ b/protocol/x/clob/keeper/orders_test.go @@ -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) + + _, _, err = ks.ClobKeeper.PlaceShortTermOrder(ctx, msg) require.NoError(t, err) } @@ -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) diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index db541650c3..d0de165e08 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -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 + } + } + makerRemainingSize, makerHasRemainingSize := m.GetOrderRemainingAmount(ctx, makerOrder.Order) if !makerHasRemainingSize { panic(fmt.Sprintf("mustPerformTakerOrderMatching: maker order has no remaining amount %v", makerOrder.Order)) diff --git a/protocol/x/clob/types/expected_keepers.go b/protocol/x/clob/types/expected_keepers.go index 872daf2905..05e25e2521 100644 --- a/protocol/x/clob/types/expected_keepers.go +++ b/protocol/x/clob/types/expected_keepers.go @@ -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 +} diff --git a/protocol/x/clob/types/mem_clob_keeper.go b/protocol/x/clob/types/mem_clob_keeper.go index 6e25cadf35..bd396a6c16 100644 --- a/protocol/x/clob/types/mem_clob_keeper.go +++ b/protocol/x/clob/types/mem_clob_keeper.go @@ -115,4 +115,5 @@ type MemClobKeeper interface { subaccountId satypes.SubaccountId, order PendingOpenOrder, ) satypes.UpdateResult + MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error } diff --git a/protocol/x/clob/types/operations_to_propose.go b/protocol/x/clob/types/operations_to_propose.go index e135987178..997197c47b 100644 --- a/protocol/x/clob/types/operations_to_propose.go +++ b/protocol/x/clob/types/operations_to_propose.go @@ -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 +}