Seismic remediations: ERC-3009 plaintext bypass+ encrypted-amount nonce scan#119
Merged
Conversation
…Approval events Align the SRC20-facing shielded events with the target shape: Transfer(address indexed from, address indexed to, bytes32 indexed encryptKeyHash, bytes encryptedAmount) Approval(address indexed owner, address indexed spender, bytes32 indexed encryptKeyHash, bytes encryptedAmount) encryptKeyHash is keccak256 of the recipient/spender's registered public key (the key encryptedAmount is encrypted to), or bytes32(0) on the no-registered-key fallback. Being indexed, it lets a client identify which registered key a ciphertext is bound to (keys are overwritable via registerPublicKey) and pick the matching private key to decrypt. _encryptAmount now returns (bytes32 encryptKeyHash, bytes ciphertext); the two shielded emit sites pass it through. JMIExtension and MYieldToOneForcedTransfer inherit the change. Updated unit + in-process seismic integration tests, the e2e topic filter, and NatSpec.
…for the infra allowlist Gate setAllowlisted behind a dedicated ALLOWLIST_MANAGER_ROLE (operator) instead of DEFAULT_ADMIN_ROLE, administered by a new ALLOWLIST_ADMIN_ROLE whose holder (allowlistAdmin) is passed into initialize. _setRoleAdmin wires ALLOWLIST_ADMIN_ROLE as the role admin of ALLOWLIST_MANAGER_ROLE, so allowlist control is delegated independently of the default admin; initialize reverts ZeroAllowlistAdmin on a zero address. The new initializer param is appended last (robust to the harness param reordering) and threaded through MYieldToOneForcedTransfer, JMIExtension, the three harnesses, the Config structs + DeployBase (init-data extracted to helpers to avoid stack-too-deep), the deploy entry scripts (ALLOWLIST_ADMIN env), and every test setUp. make tests: 486 passed. make integration-seismic: 22 passed.
The six ERC-3009 transferWithAuthorization/receiveWithAuthorization overloads inherited from ERC3009Upgradeable reach MExtension._transfer directly, emitting a plaintext Transfer(address,address,uint256) with the cleartext amount and bypassing the encrypted-event machinery — an ungated, fund-moving path that leaks the amount the shielded SRC-20 otherwise hides. Inherited verbatim by MYieldToOneForcedTransfer (deployed) and JMIExtension. Override all six to revert UseShieldedTransfer, mirroring the existing permit reverts. ERC-3009 hard-codes a plaintext uint256 value, so the path cannot be shielded — reverting is the only privacy-preserving option. Needs the matching virtual on the base entry points: lib/common feat/erc20-virtual bumped a1fbf37..703ba8a. Tests: revert coverage for all six overloads incl. valid-signature cases proving even a valid authorization reverts, plus an inherited-revert regression on MYieldToOneForcedTransfer. JMIExtension 23,067 B (< 24,576 EIP-170).
…nce, not scan it The encrypted Transfer/Approval payload's AES-GCM nonce is bytes12(keccak256(from, to, ++encryptedEventNonce)), but the counter was never emitted, forcing the off-chain decryptor to brute-force scan 1..N per event. Emit a dedicated EncryptedAmountNonce(from, to, nonce) from _encryptAmount whenever a counter is consumed (registered-key path only, never the no-key fallback or the plaintext infra paths), leaving the existing Transfer/Approval topic0 untouched. The decryptor reads the exact counter from the event (--nonce-counter); --scan stays as a legacy fallback for pre-event logs. Zero-amount shielded transfers early-return as a no-op (no emit, no counter, no balance change); the zero-amount tests assert this and the nonce/ciphertext fuzz tests are bounded to amount >= 1. ConfigureSeismicExtension now asserts each infra address is allowlisted after configuration. make tests: 494 passed. make integration-seismic: 22 passed.
Khrafts
changed the title
Draft
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.
Two audit remediations for the Seismic shielded SRC-20 (
MYieldToOneand its inheritorsMYieldToOneForcedTransfer— deployed on chain 5124 — andJMIExtension).Finding #1 — HIGH — ERC-3009 plaintext transfer bypass
The six inherited
transferWithAuthorization/receiveWithAuthorizationoverloads reachedMExtension._transferdirectly, emitting a plaintextTransfer(address,address,uint256)with the cleartext amount and bypassing the encrypted-event machinery — an ungated, fund-moving path that leaked the amount the shielded token otherwise hides. Inherited verbatim by the deployedMYieldToOneForcedTransferand byJMIExtension.Fix: override all six to
revert UseShieldedTransfer()(mirroring the existingpermitreverts). ERC-3009 hard-codes a plaintextuint256 value, so the path cannot be shielded — reverting is the only privacy-preserving option.Submodule dependency: requires
virtualon the base entry points.lib/commonis bumpeda1fbf37 → 703ba8a(feat: mark ERC3009 transfer/receive entry points virtualonfeat/erc20-virtual, already pushed to origin).Finding #2 — MEDIUM — encrypted-amount nonce required a brute-force scan
The AES-GCM nonce is
bytes12(keccak256(from, to, ++encryptedEventNonce)), but the counter was never emitted — forcing the off-chain decryptor to scan1..Nper event.Fix: emit a dedicated
EncryptedAmountNonce(from, to, nonce)from_encryptAmountwhenever a counter is consumed (registered-key path only; never the no-key fallback or the plaintext infra paths), leaving the existingTransfer/Approvaltopic0untouched. The decryptor reads the exact counter from the event (--nonce-counter);--scanstays as a legacy fallback for pre-event logs. Zero-amount shielded transfers early-return as a no-op.Also included
This branch also carries two prior commits not yet on
feat/seismic:59bbe3bfeat(seismic): indexedencryptKeyHashtopic on the shielded events42c8631feat(MYieldToOne):ALLOWLIST_MANAGER_ROLE+ALLOWLIST_ADMIN_ROLETesting
make tests: 494 passedmake integration-seismic: 22 passed (in-process on-chain → off-chain round-trip decrypt)JMIExtensionwithin the EIP-170 24,576 B limit