-
Notifications
You must be signed in to change notification settings - Fork 4
feat: ID-4134: Support Bootstrap Flow for Wallet Initial Transaction #74
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: main
Are you sure you want to change the base?
Conversation
Hook into signature validation to bootstrap wallet deployment for first txn without user signature.
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 13
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
hardhat.config.ts
Outdated
| compilers: [{ | ||
| version: '0.8.17', | ||
| settings: { | ||
| viaIR: true, |
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.
@drinkcoffee I tweaked this settling to resolve a "stack too deep" error. Not sure if there's any unintended side effects that I'm not aware of. Pls advise.
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.
@drinkcoffee I ended up changing this because it caused issues with the existing tests where revert messages were validated. I had to introduce an inline struct to reduce the number of variables on the stack.
remove usage of viaIR & update logic to workaround deep stack issue
| uint256 rindex // read index | ||
| ) = _signature.readFirstUint16(); | ||
|
|
||
| SignatureValidationState memory state = SignatureValidationState({ |
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.
Storing these values in memory rather than on the stack will make the code more expensive to run
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.
Had to go down this path to workaround the "stack too deep" issue 🤔 Any suggestions on alternate approaches?
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.
Use local variables and change the FLAG_DYNAMIC_SIGNATURE block to:
// Read signer
(addr, rindex) = _signature.readAddress(rindex);
bytes memory signature;
{
// Read signature size
uint256 size;
(size, rindex) = _signature.readUint16(rindex);
// Read dynamic size signature
(signature, rindex) = _signature.readBytes(rindex, size);
}
require(isValidSignature(_hash, addr, signature), "ModuleAuthDynamic#_signatureValidation: INVALID_SIGNATURE");
Doing this helps the compiler by providing scope to size.
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.
I wasn't able to resolve the stack issue with block scoping, have retained the memory variable approach for now.
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.
When I retried, I couldn't get that specific fix to work. I am not sure why.
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.
I have created a PR which takes an alternative approach to allow the variables to be on the stack rather than in memory. Approve it and merge it into this one if you are happy with the approach.
#76
| return (true, true, state.imageHash); | ||
| } | ||
|
|
||
| (bool verified, bool needsUpdate) = _isValidImage(state.imageHash); |
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.
The CFA will match the calculated image hash: a combination of the user key's address and the Immutable key's address. When an alternative user secret provider is used, the user's key will be different, so the address will be different, and hence the calculated image hash and then CFA will be different.
If this is the case, then the _isValidImage check will fail here: https://github.com/immutable/wallet-contracts/blob/main/src/contracts/modules/commons/ModuleAuthDynamic.sol#L35
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.
What am I missing in my understanding?
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.
In the scenario where a user's CFA was generated with signer A but the user is now associated with signer B (e.g., due to using different identity providers on different chains), the _isValidImage check will not be invoked on first transaction.
Instead, during the first transaction (when currentNonce == 1), the contract performs an early exit after validating that the Immutable Signer is present among the signers.
At line 158-160:
if (currentNonce == 1 && state.immutableSignerContractFound) {
return (true, true, state.imageHash);
}
This bootstrap flow bypasses the _isValidImage check entirely. The computed image hash (derived from the new signer configuration, including signer B) is returned and subsequently stored via updateImageHashInternal().
This is the intentional design of the bootstrap flow - it allows the Immutable Signer to initialize the wallet with a different user key configuration while preserving the original CFA.
The _isValidImage check at line 162 is only reached for subsequent transactions (nonce ≥ 2), where it validates against the already-stored image hash.
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.
That makes sense. Thank you for the detailed explanation.
| 2. Any signature that includes `address(0)` would trigger bootstrap | ||
|
|
||
| **Mitigation:** | ||
| - Constructor validates that `_immutableSignerContract != address(0)` |
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.
We validate that the immutableSignerContract address is non-zero, but should we also validate that the actual primarySigner address value is non-zero as well?
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.
Yes probably should have but unfortunately the signer contract is non upgradable. We however do have the ability to rotate the signer EOA.
|
|
||
| **Mitigation:** | ||
| - ImmutableSigner uses role-based access control for key rotation | ||
| - Key rotation can be performed without affecting wallet addresses |
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.
Oh do we already have key rotation in place for the Immutable Signer private key?
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.
Yes we do and we also support a rollover period.
|
|
||
| **Residual Risk:** MEDIUM - Key compromise would allow unauthorized bootstrap | ||
|
|
||
| **Detection:** Monitor `PrimarySignerUpdated` events and validate bootstrap transactions against expected patterns. |
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.
I don't think monitoring PrimarySignerUpdated would allow us to detect if the ImmutableSigner was compromised? As the attacker wouldn't update the signer in this case.
Though this would allow us to detect if the SIGNER_ADMIN key was compromised.
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.
You're right, the private key of the Immutable signer being compromised will not be detected via the suggested monitoring. I'll fix it up.
| }) | ||
| }) | ||
| }) | ||
|
|
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.
I think we can add tests to validate the bootstrap flow (and subsequent transactions) works if the immutable signer is updated (and doesn't work using the old signer address).
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.
I don't think there is value in testing the signer key rotation as part of this work. The bootstrap flow doesn't actually validate the signature, it only extracts the signer information from the signature and attempts to match it to the Immutable signer contract address. The contract address itself is fixed and not affected by signer rotation.
| 2. Sign bootstrap transactions for any wallet | ||
| 3. Take control of user wallets during their first transaction | ||
|
|
||
| **Mitigation:** |
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.
key rotation needs startup implementation contract to point to a new version?
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.
The Immutable signer contract's primary signer can be rotated without impacting the CFA address or other wallet workflow. The implementation contract only has the Immutable Signer's contract address persisted in storage so key rotation doesn't require any update to the implementation contract either.
Eliminate the clock skew issue between the JavaScript runtime and the blockchain using blockchain timestamp
| (flag, addrWeight, state.rindex) = _signature.readUint8Uint8(state.rindex); | ||
|
|
||
| if (flag == FLAG_ADDRESS) { | ||
| // Read plain address |
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.
I think we only use FLAG_SIGNATURE and FLAG_DYNAMIC_SIGNATURE. If I am correct, then we should remove the FLAG_ADDRESS option.
At attacker could create two valid signatures that are FLAG_SIGNATURE, and include the Immutable signer as a FLAG_ADDRESS. The checks below would deem the signature valid and bootstrap the wallet at line 109 and 120.
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.
They don't need to create two valid signatures, as we don't check the weight. Hence, they could just create a _signature block that had Immutable signer as a FLAG_ADDRESS, and then they could set the image hash to a value of their choosing.
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.
You're right @drinkcoffee
This is a critical bug. I've updated the contract with the following changes:
- Removed FLAG_ADDRESS support - This prevents attackers from including the immutable signer address in the signature without actually providing a valid signature from that address.
- Required verified signatures from the Immutable signer contract - The immutable signer must now actually sign the transaction (via FLAG_SIGNATURE or FLAG_DYNAMIC_SIGNATURE) rather than just being listed as an address. The immutableSignerContractSigned flag is only set to true after the signature has been cryptographically verified.
These changes ensure that the bootstrap flow can only be triggered when the Immutable signer contract has genuinely signed the transaction, preventing unauthorized bootstrap attacks.
Fix bootstrap signature validation to require verified signature from Immutable signer and meet weight threshold, removing FLAG_ADDRESS support to prevent unauthorized wallet takeover on first transaction.
Summary:
This PR implements support for the bootstrap flow in the v1 wallet system. This is done by checking if the initial txn is signed by the Immutable signer without requiring a pre-stored image hash.
Motivation:
Pre-change:
When a new wallet is created, the image hash (representing the wallet's signer configuration) needs to be stored before transactions can be validated. In the absence of the imageHash, the first transaction is validated by recalculating the CREATE2 address of the wallet using the imageHash as the salt. The imageHash is retrieved from the signed message and the message payload. If the calculated address (CFA) matches the deployed wallet address then the imageHash is stored and used to validate subsequent transactions.
The problem
Currently in the Immutable ZKEVM chain, the primary wallet owner / signer identity instrument is stored by a 3rd party and we access the signer via their TEE. In a multi-chain world, we would potentially be integrating with different infrastructure providers across chain and therefore have different signers.
The primary wallet owner / signer influences the CFA i.e the user's Passport wallet address and since we need to preserve the same CFA, the current model of validating the first transaction by verifying if the recalculated wallet CFA matches the deployed wallet address doesn't work.
Post-change
To work around the above problem, this PR introduces the following changes:
Overall Flow:

Note
Introduces immutable signer–based bootstrap to validate the first wallet transaction without user signature, updating constructors, deployments, and compiler settings.
ModuleAuthDynamic: AddsIMMUTABLE_SIGNER_CONTRACTand overrides signature validation to auto-approve first tx (nonce=0) when immutable signer is present; integrates nonce/image checks viaModuleStorage/NonceKey.MainModuleDynamicAuth+ mocks (MainModuleMockV1/2/3): Constructors now accept and pass_immutableSignerContract.ModuleAuth: ExposesFLAG_*asinternal; marks validation functionsvirtualto enable overrides.scripts/deploy.ts&scripts/step4.ts: DeployImmutableSignerbeforeMainModuleDynamicAuthand supply its address during deployment; output includes immutable signer details.hardhat.config.ts: EnablesviaIRin Solidity compiler settings..gitignore: Ignoreslib/.Written by Cursor Bugbot for commit 7716f65. This will update automatically on new commits. Configure here.