fix(transaction-pay-controller): guard fiat post-ramp second leg against locked keyring and CHOMP races#9267
Open
matthewwalsh0 wants to merge 10 commits into
Open
fix(transaction-pay-controller): guard fiat post-ramp second leg against locked keyring and CHOMP races#9267matthewwalsh0 wants to merge 10 commits into
matthewwalsh0 wants to merge 10 commits into
Conversation
…post-ramp second leg
matthewwalsh0
force-pushed
the
fix/pay-wait-keyring-unlock
branch
from
1a62b29 to
e3d50fc
Compare
10 tasks
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
…SD vault deposit
Adds CHOMP (auto-vault) detection to submitDirectMusdVaultDeposit:
- Pre-check via eth_getLogs before addTransactionBatch; returns CHOMP hash
without adding a child transaction when CHOMP has already vaulted the funds.
- Post-check in the catch path to handle the race where CHOMP wins between
pre-check and submit (e.g. the EIP-7702 account-support error case).
- Errors in either CHOMP check are swallowed so they never break the normal
vault submit path.
CHOMP baseline block is derived from the ramps settlement tx receipt
blockNumber already fetched in getTransferredAmountFromTxHash (ERC-20 path),
requiring no extra network request. resolveSourceAmountRaw now returns
{ amountRaw, chompFromBlock } and getTransferredAmountFromTxHash returns
{ amountRaw, blockNumber }.
Detection: single eth_getLogs call — mUSD Transfer(from=moneyAccount) with
amount >= sourceAmountRaw; newest log wins.
matthewwalsh0
changed the title
…indChompDeposit helper, rename chompFromBlock to fromBlock
… log into chomp util
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f307c21. Configure here.
| txHash: txLog.transactionHash, | ||
| }); | ||
|
|
||
| return txLog.transactionHash; |
There was a problem hiding this comment.
CHOMP scan matches unrelated transfers
High Severity
CHOMP idempotency treats any recent mUSD Transfer from the Money Account with amount at least the settled raw total as success. That can match unrelated outbound sends or another deposit’s vault tx, so the direct vault batch is skipped while this order’s ramp credit may stay unvaulted.
Reviewed by Cursor Bugbot for commit f307c21. Configure here.
| decimals: tokenInfo.decimals, | ||
| }); | ||
|
|
||
| return { amountRaw, fromBlock: undefined }; |
There was a problem hiding this comment.
Settlement block lost on fallback
Medium Severity
When resolveSourceAmountRaw reads a settlement receipt but cannot derive an on-chain inbound amount, it falls back to order.cryptoAmount and drops the receipt blockNumber, so fromBlock stays undefined and CHOMP pre/post checks are skipped even though the baseline block was already fetched.
Reviewed by Cursor Bugbot for commit f307c21. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Two failure modes for the direct mUSD Money Account vault deposit are addressed in this PR.
Locked keyring at submit time
When a fiat order completes and the post-ramp second leg is triggered (direct mUSD vault deposit, Relay nested-calldata, or simple relay), the wallet may be locked — for example if the user backgrounded the app during Apple Pay checkout and the auto-lock timer fired. In that state
KeyringController.keyringsis empty,doesAccountSupportEIP7702returnsfalsefor the Money Account address, andaddTransactionBatch(called withdisableHookanddisableSequential) throwsAccount does not support EIP-7702, failing the deposit.A
waitForKeyringUnlockguard is added at the top ofsubmitRelayAfterFiatCompletioninfiat-submit.ts. Before any second-leg logic runs the function checksKeyringController:getState().isUnlocked. If already unlocked it proceeds immediately; if locked it subscribes toKeyringController:unlockand waits indefinitely, resuming only after the user authenticates.KeyringControllerUnlockEventis added toAllowedEventsintypes.tsto allow the subscription.CHOMP auto-vault race
CHOMP is a backend service that may auto-vault mUSD from the Money Account independently of the extension/mobile submit path. If CHOMP runs concurrently with the checkout flow the vault deposit that
addTransactionBatchtries to submit may conflict with a deposit CHOMP already completed, causing a double-vault.A CHOMP idempotency check is added in
submitDirectMusdVaultDepositusing a singleeth_getLogscall that scans for recent mUSD Transfer-out events from the Money Account. The baseline block is derived from the ramps settlement tx receipt (already fetched for the amount) so no additional network request is needed. The check runs beforeaddTransactionBatch(skip the submit entirely) and again in the catch path (CHOMP may have won the race just before submit). Detection errors are swallowed so they never break the normal vault path.References
Checklist
Note
Medium Risk
Changes the fiat post-ramp and direct mUSD vault submission path (signing, batch submit, and on-chain idempotency) and adds a new messenger event permission for clients; incorrect CHOMP matching could skip or mis-attribute a vault hash.
Overview
Fixes two post–fiat-ramp failure modes for the direct mUSD Money Account vault path and other fiat second legs.
Locked keyring: After the on-ramp order completes, the controller now calls
waitForKeyringUnlockbeforesubmitRelayAfterFiatCompletion(relay, nested calldata, and direct mUSD). If the wallet is locked it subscribes toKeyringController:unlockand resumes only after unlock, avoiding false EIP-7702 /addTransactionBatchfailures when the user was away during checkout.KeyringControllerUnlockEventis added toAllowedEventsso clients must grant that subscription.CHOMP race: New
findRecentChompVaultDepositscans Monadeth_getLogsfor mUSDTransferevents from the Money Account (newest first, amount ≥ settled raw).submitDirectMusdVaultDepositruns this beforeaddTransactionBatch(skip submit and return the CHOMP tx hash) and again on batch failure (treat CHOMP as success). Scan errors are swallowed so the normal vault path still runs. The log window starts at the ramps settlement receipt block:resolveSourceAmountRaw/getTransferredAmountFromTxHashnow return{ amountRaw, fromBlock }/{ amountRaw, blockNumber }instead of a bare string, with no extra RPC when the receipt was already fetched for ERC-20 amounts.Reviewed by Cursor Bugbot for commit f307c21. Bugbot is set up for automated code reviews on this repo. Configure here.