fix(agglayer): validate decoded AccountId in eth_address::to_account_id#3231
Draft
partylikeits1983 wants to merge 3 commits into
Draft
fix(agglayer): validate decoded AccountId in eth_address::to_account_id#3231partylikeits1983 wants to merge 3 commits into
AccountId in eth_address::to_account_id#3231partylikeits1983 wants to merge 3 commits into
Conversation
The bridge-in claim decodes a deposit leaf's destinationAddress into a Miden AccountId via eth_address::to_account_id and routes bridged value to it (P2ID for the native faucet, or a MINT note that later produces a P2ID). The conversion only checked that the most-significant 4 bytes are zero, that the limbs are u32, and that the packed words fit in the field - it never enforced the AccountId structural invariants. An embedded destination that decodes into a structurally invalid AccountId (e.g. a non-zero suffix least-significant byte) was accepted and emitted as the output target, producing a P2ID/MINT note that no valid account can consume and thus permanently locking the bridged assets. The Rust reference conversion (EthEmbeddedAccountId::try_from_eth_address) already rejects such payloads, so the on-chain path was strictly more permissive. Validate the decoded id with account_id::validate inside to_account_id, matching the Rust contract and the kernel's own usage. Add PoC tests covering the non-embedded trap and the invalid-embedded rejection, and load the protocol/standards libraries in the standalone agglayer MASM test host so agglayer procedures that call into them resolve at runtime.
partylikeits1983
changed the title
AccountId in eth_address::to_account_id
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.
Note: discovered by OZ AI audit tool
The bridge-in claim flow decodes a deposit's
destinationAddressinto a MidenAccountIdviaeth_address::to_account_idand uses that id as theAccountIdtarget of the emitted P2ID note.The conversion only checked the byte layout (top 4 bytes zero, u32 limbs, packed words fit in the field) and never checked the
AccountIdstructural invariants. As a result, an embedded address that decodes into a structurally invalidAccountId(for example, a non-zero suffix least-significant byte) was accepted and emitted as the output target, a note that no valid account could ever consume.The Rust conversion (
EthEmbeddedAccountId::try_from_eth_address) already validates the decoded id, so the on-chain MASM path was more permissive than the reference.Fix
Validate the decoded id with
account_id::validateinsideto_account_id, matching the Rust contract and the transaction kernel's own usage.to_account_idhas a single caller, so the conversion now always yields a well-formedAccountId.Tests
Added tests for the non-embedded trap and the invalid-embedded rejection (the latter fails before the fix and passes after). All agglayer tests pass, including the full claim flow.