-
Notifications
You must be signed in to change notification settings - Fork 302
fix(sdk-core): add EVM staking types to TSS signing bypass list #8658
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -1,4 +1,12 @@ | ||
| import { BaseCoin, BitGoBase, common, MPCAlgorithm, MultisigType, multisigTypes } from '@bitgo/sdk-core'; | ||
| import { | ||
| BaseCoin, | ||
| BitGoBase, | ||
| common, | ||
| MPCAlgorithm, | ||
| MultisigType, | ||
| multisigTypes, | ||
| NO_RECIPIENT_TX_TYPES, | ||
| } from '@bitgo/sdk-core'; | ||
| import { BaseCoin as StaticsBaseCoin, coins } from '@bitgo/statics'; | ||
| import { | ||
| AbstractEthLikeNewCoins, | ||
|
|
@@ -68,10 +76,7 @@ export class Bsc extends AbstractEthLikeNewCoins { | |
| const { txParams, txPrebuild, wallet } = params; | ||
| if ( | ||
| !txParams?.recipients && | ||
| !( | ||
| txParams.prebuildTx?.consolidateId || | ||
| (txParams.type && ['acceleration', 'fillNonce', 'transferToken', 'tokenApproval'].includes(txParams.type)) | ||
| ) | ||
| !(txParams.prebuildTx?.consolidateId || (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type))) | ||
|
Contributor
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. [Critical] This guard still throws for BSC staking transactions. After
The staking types propagated via Possible fixes:
|
||
| ) { | ||
| throw new Error(`missing txParams`); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ import { PopulatedIntent, TxRequest } from './baseTypes'; | |
|
|
||
| /** | ||
| * Transaction types that legitimately carry no explicit recipients. | ||
| * verifyTransaction handles no-recipient validation for these internally. | ||
| * These are non-staking ECDSA types where verifyTransaction handles | ||
| * no-recipient validation internally. | ||
| * Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction. | ||
| */ | ||
| export const NO_RECIPIENT_TX_TYPES = new Set([ | ||
|
|
@@ -15,7 +16,7 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ | |
| 'consolidate', | ||
| 'bridgeFunds', | ||
| 'enableToken', | ||
| 'customTx', // DeFi/WalletConnect smart contract interactions have no traditional recipients | ||
| 'customTx', | ||
| ]); | ||
|
|
||
| /** | ||
|
|
@@ -25,8 +26,13 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ | |
| * (native amount = 0, so buildParams is empty). Falls back to intent recipients | ||
| * mapped to ITransactionRecipient shape when txParams.recipients is absent. | ||
| * | ||
| * Staking intents (BSC delegate/undelegate, CELO stake/unstake, etc.) are | ||
| * identified generically by the presence of `stakingRequestId` on the intent — | ||
| * a required field on BaseStakeIntent in @bitgo/public-types. These intents | ||
| * have no txParams recipients by design; validation is done at the coin layer. | ||
| * | ||
| * Throws InvalidTransactionError if no recipients can be resolved and the | ||
| * transaction type is not a known no-recipient type. | ||
| * transaction is not a known no-recipient type. | ||
| */ | ||
| export function resolveEffectiveTxParams( | ||
| txRequest: TxRequest, | ||
|
|
@@ -43,7 +49,21 @@ export function resolveEffectiveTxParams( | |
| recipients: txParams?.recipients?.length ? txParams.recipients : intentRecipients, | ||
| }; | ||
|
|
||
| if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(effectiveTxParams.type ?? '')) { | ||
| // Fall back to intent.intentType when txParams.type is not explicitly set. | ||
| // Staking wallets call signTransaction without txParams, so the type lives only in the intent. | ||
| const txType = effectiveTxParams.type ?? (txRequest.intent as PopulatedIntent)?.intentType ?? ''; | ||
|
|
||
| // Propagate the resolved type so downstream callers (e.g. verifyTssTransaction) can use it. | ||
| if (!effectiveTxParams.type && txType) { | ||
| effectiveTxParams.type = txType; | ||
| } | ||
|
|
||
| // All staking intents (BSC delegate/undelegate, CELO stake/unstake, etc.) carry | ||
| // stakingRequestId as a required field on BaseStakeIntent (@bitgo/public-types). | ||
| // Use its presence as a generic staking signal — no need to enumerate every intentType. | ||
| const isStakingIntent = !!(txRequest.intent as any)?.stakingRequestId; | ||
|
Contributor
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. [High] Consider importing function isBaseStakeIntent(intent: unknown): intent is BaseStakeIntent {
return typeof (intent as any)?.stakingRequestId === 'string';
}
const isStakingIntent = isBaseStakeIntent(txRequest.intent);Or extend the |
||
|
|
||
| if (!effectiveTxParams.recipients?.length && !isStakingIntent && !NO_RECIPIENT_TX_TYPES.has(txType)) { | ||
|
Contributor
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. [Critical] This fix prevents The full call chain after this function returns for a staking intent (
The fix moves the error from |
||
| throw new InvalidTransactionError( | ||
| 'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.' | ||
| ); | ||
|
|
||
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.
[Critical] Same issue as in
bsc.ts:NO_RECIPIENT_TX_TYPESdoes not contain staking types (delegate,undelegate,stake,unstake, etc.). When a staking transaction arrives here withtxParams = { type: 'delegate' }(returned byresolveEffectiveTxParams), this check throws'missing txParams'.Note that this parent class is also what
bsc.tswould fall through to if the override were removed (as the commit message for467ff30dstates). Either way, the staking bypass needs to be present at this layer too.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.
@mrdanish26 let's also add the
stakingRequestId, it should be in theprebuildTxobject. @noel-bitgo if you can confirm.