Skip to content

Commit 0b1fa04

Browse files
fix: Predict withdraw beforeSign handling (#29968)
## Summary - Guards Predict withdraw `beforeSign` so it only signs the active withdraw transaction. - Skips signing when the withdraw calldata is already Safe execution calldata produced by the MetaMask Pay follow-up flow. - Reuses the active withdraw state consistently when updating transaction params. - Adds unit coverage for stale transaction IDs and already-signed Safe calldata. ## Changelog CHANGELOG entry: Fixed Predict withdraw signing when withdraw transaction calldata is already prepared. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches transaction pre-signing for Predict withdrawals, which can affect what data gets signed and where it is sent. Changes are well-scoped with added unit coverage, but mistakes could block withdrawals or sign incorrect transactions. > > **Overview** > Predict withdraw `beforeSign` now only runs `signWithdraw` for the **currently active** withdraw transaction (by `transactionId`) and reuses that active state when setting `to`/updating tx params. > > It also **skips signing** when the nested withdraw calldata is not an ERC-20 `transfer` selector (e.g., already-prepared Safe execution calldata from MetaMask Pay), and adds unit tests covering stale transaction IDs, pre-signed calldata pass-through, and updated call-data expectations. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5681381. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 4997055 commit 0b1fa04

2 files changed

Lines changed: 89 additions & 7 deletions

File tree

app/components/UI/Predict/controllers/PredictController.test.ts

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6361,6 +6361,9 @@ describe('PredictController', () => {
63616361
});
63626362

63636363
describe('beforeSign', () => {
6364+
const ERC20_TRANSFER_CALL_DATA =
6365+
'0xa9059cbb00000000000000000000000012345678901234567890123456789012345678900000000000000000000000000000000000000000000000000000000000011170' as const;
6366+
63646367
const mockTransactionMeta = {
63656368
id: 'tx-1',
63666369
txParams: {
@@ -6373,7 +6376,7 @@ describe('PredictController', () => {
63736376
{
63746377
id: 'nested-1',
63756378
type: 'predictWithdraw' as const,
6376-
data: '0xoriginaldata' as `0x${string}`,
6379+
data: ERC20_TRANSFER_CALL_DATA,
63776380
},
63786381
],
63796382
};
@@ -6424,6 +6427,63 @@ describe('PredictController', () => {
64246427
});
64256428
});
64266429

6430+
it('return undefined when transaction does not match active withdraw transaction id', async () => {
6431+
await withController(async ({ controller }) => {
6432+
controller.updateStateForTesting((state) => {
6433+
state.withdrawTransaction = {
6434+
chainId: 137,
6435+
status: PredictWithdrawStatus.IDLE,
6436+
providerId: POLYMARKET_PROVIDER_ID,
6437+
predictAddress: '0xPredict' as `0x${string}`,
6438+
transactionId: 'tx-1',
6439+
amount: 0,
6440+
};
6441+
});
6442+
6443+
const result = await controller.beforeSign({
6444+
transactionMeta: {
6445+
...mockTransactionMeta,
6446+
id: 'tx-2',
6447+
} as any,
6448+
});
6449+
6450+
expect(result).toBeUndefined();
6451+
expect(mockPolymarketProvider.signWithdraw).not.toHaveBeenCalled();
6452+
});
6453+
});
6454+
6455+
it('return undefined when predict withdraw calldata is already signed safe execution data', async () => {
6456+
await withController(async ({ controller }) => {
6457+
controller.updateStateForTesting((state) => {
6458+
state.withdrawTransaction = {
6459+
chainId: 137,
6460+
status: PredictWithdrawStatus.IDLE,
6461+
providerId: POLYMARKET_PROVIDER_ID,
6462+
predictAddress: '0xPredict' as `0x${string}`,
6463+
transactionId: 'tx-1',
6464+
amount: 0,
6465+
};
6466+
});
6467+
6468+
const result = await controller.beforeSign({
6469+
transactionMeta: {
6470+
...mockTransactionMeta,
6471+
nestedTransactions: [
6472+
{
6473+
id: 'nested-1',
6474+
type: 'predictWithdraw' as const,
6475+
data: '0x6a76120200000000' as `0x${string}`,
6476+
},
6477+
],
6478+
} as any,
6479+
});
6480+
6481+
expect(result).toBeUndefined();
6482+
expect(mockPolymarketProvider.signWithdraw).not.toHaveBeenCalled();
6483+
expect(mockInvalidateQueries).not.toHaveBeenCalled();
6484+
});
6485+
});
6486+
64276487
it('return undefined when provider does not support signWithdraw', async () => {
64286488
await withController(async ({ controller }) => {
64296489
controller.updateStateForTesting((state) => {
@@ -6470,7 +6530,7 @@ describe('PredictController', () => {
64706530
});
64716531

64726532
expect(mockPolymarketProvider.signWithdraw).toHaveBeenCalledWith({
6473-
callData: '0xoriginaldata',
6533+
callData: ERC20_TRANSFER_CALL_DATA,
64746534
signer: expect.objectContaining({
64756535
address: '0x1234567890123456789012345678901234567890',
64766536
signTypedMessage: expect.any(Function),

app/components/UI/Predict/controllers/PredictController.ts

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,8 @@ const MESSENGER_EXPOSED_METHODS = [
389389
'trackShareAction',
390390
] as const;
391391

392+
const ERC20_TRANSFER_FUNCTION_SELECTOR = '0xa9059cbb';
393+
392394
/**
393395
* PredictController - Protocol-agnostic prediction markets trading controller
394396
*
@@ -2753,7 +2755,18 @@ export class PredictController extends BaseController<
27532755
}
27542756
| undefined
27552757
> {
2756-
if (!this.state.withdrawTransaction) {
2758+
const activeWithdrawTransaction = this.state.withdrawTransaction;
2759+
2760+
if (!activeWithdrawTransaction) {
2761+
return;
2762+
}
2763+
2764+
if (
2765+
activeWithdrawTransaction.transactionId &&
2766+
request.transactionMeta.id !== activeWithdrawTransaction.transactionId
2767+
) {
2768+
// MetaMask Pay creates a follow-up transaction after the original withdraw
2769+
// is signed. Only the active withdraw transaction should be signed here.
27572770
return;
27582771
}
27592772

@@ -2774,12 +2787,21 @@ export class PredictController extends BaseController<
27742787

27752788
const signer = this.getSigner(request.transactionMeta.txParams.from);
27762789

2777-
const chainId = this.state.withdrawTransaction.chainId;
2790+
const chainId = activeWithdrawTransaction.chainId;
27782791

27792792
const networkClientId = this.messenger.call(
27802793
'NetworkController:findNetworkClientIdByChainId',
27812794
numberToHex(chainId),
27822795
);
2796+
const withdrawDataPrefix = withdrawTransaction.data?.slice(0, 10);
2797+
2798+
if (
2799+
withdrawDataPrefix?.toLowerCase() !== ERC20_TRANSFER_FUNCTION_SELECTOR
2800+
) {
2801+
// signWithdraw expects the original ERC20 transfer calldata. Pay-created
2802+
// batches may already contain signed Safe calldata and should pass through.
2803+
return;
2804+
}
27832805

27842806
// Invalidate query cache (to avoid nonce issues)
27852807
await this.invalidateQueryCache(chainId);
@@ -2793,7 +2815,7 @@ export class PredictController extends BaseController<
27932815
...withdrawTransaction,
27942816
from: request.transactionMeta.txParams.from,
27952817
data: callData,
2796-
to: this.state.withdrawTransaction?.predictAddress as Hex,
2818+
to: activeWithdrawTransaction.predictAddress as Hex,
27972819
};
27982820

27992821
// Attempt to estimate gas for the updated transaction
@@ -2833,8 +2855,8 @@ export class PredictController extends BaseController<
28332855
return {
28342856
updateTransaction: (transaction: TransactionMeta) => {
28352857
transaction.txParams.data = callData;
2836-
transaction.txParams.to = this.state.withdrawTransaction
2837-
?.predictAddress as Hex;
2858+
transaction.txParams.to =
2859+
activeWithdrawTransaction.predictAddress as Hex;
28382860
transaction.assetsFiatValues = {
28392861
...transaction.assetsFiatValues,
28402862
receiving: String(amount),

0 commit comments

Comments
 (0)