Conversation
paulbalaji
left a comment
There was a problem hiding this comment.
Follow-up Review (post-fix commits)
The fix commits at 4c801d7 addressed all previously raised issues — nice work. Two new items remain.
Prior Issues — All Resolved
Confirmed fixed: read-only domain-PDA rate-limit bypass, getter/handle identity leak (now passes None with clear comment at line 345-347), verify-account-meta dedup mismatch, duplicate validator validation, CLI RateLimited round-trip, Routing.default_ism fallback metas, and missing system-program validation in set_domain_ism.
| let additional_signers: Vec<&SealevelKeypair> = self | ||
| .get_signer_if_separate() | ||
| .filter(|signer| { | ||
| payload | ||
| .instruction | ||
| .accounts | ||
| .iter() | ||
| .any(|meta| meta.pubkey == signer.pubkey() && meta.is_signer) | ||
| }) | ||
| .into_iter() | ||
| .collect(); | ||
| let tx = self | ||
| .provider | ||
| .build_estimated_tx_for_instruction( |
There was a problem hiding this comment.
HIGH: Relayer co-signs verify for any recipient-selected ISM
The getter/handle identity leak is fixed (line 345 now passes None), but the verify path still exposes the identity to any ISM the recipient picks:
get_process_payload()(line ~347) callsget_recipient_ism()— the recipient controls which ISM address is returnedget_ism_verify_account_metas()(line 254) passesidentitytosanitize_dynamic_accountsfor that ISM, preserving signer authority- Here at line 577-590,
process()adds the identity as anadditional_signerif it appears signer-marked in the final instruction
A malicious recipient can point at a custom ISM program whose VerifyAccountMetas returns the relayer identity as a signer. The relayer then co-signs the process tx, and the malicious ISM (reached via mailbox CPI) could transfer SOL out of the identity account.
Fix options:
- Only pass
identityfor ISMs that return a knownModuleType(e.g.Composite), since TrustedRelayer only exists in the composite ISM program - Allowlist specific ISM program IDs that may request the identity signer
- Only co-sign if the ISM program ID matches a configured composite ISM address
There was a problem hiding this comment.
actually as I'm thinking about it we dont need this. The identity keypair is an additional keypair next to the actual fee payer keypair which holds no funds at all and simply signs to proof the relayer identity. So even if a malicious ism obtains a co-signature, it can steal no funds or could do anything with it. Added more detailed comments in the code regarding this
There was a problem hiding this comment.
would be interested in @tkporter opinion here
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8528 +/- ##
=======================================
Coverage 79.23% 79.23%
=======================================
Files 143 143
Lines 4242 4242
Branches 428 428
=======================================
Hits 3361 3361
Misses 853 853
Partials 28 28
🚀 New features to boost your workflow:
|
| let mut packed_count = 0usize; | ||
| let mut final_results: Vec<Option<Vec<u8>>> = vec![None; ism_count]; | ||
|
|
||
| for (i, (is_null, result)) in sub_results.iter().enumerate() { | ||
| if packed_count >= threshold_usize { | ||
| break; | ||
| } | ||
| if !is_null { | ||
| if let Some(bytes) = result { | ||
| final_results[i] = Some(bytes.clone()); | ||
| packed_count = packed_count.saturating_add(1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if packed_count < threshold_usize { | ||
| for (i, (is_null, result)) in sub_results.iter().enumerate() { | ||
| if packed_count >= threshold_usize { | ||
| break; | ||
| } | ||
| if *is_null { | ||
| if let Some(bytes) = result { | ||
| final_results[i] = Some(bytes.clone()); | ||
| packed_count = packed_count.saturating_add(1); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 Aggregation metadata builder may select a failing Null sub-ISM over a passing one
In sealevel_composite.rs:196-223, the aggregation metadata builder packs Null sub-ISMs as fallback padding when non-Null sub-ISMs don't meet threshold. It iterates sub_results in order and picks the first Null sub-ISMs it finds. Since MetadataSpec::Null carries no state (paused/unpaused, accept/reject), the relayer cannot distinguish a Pausable{paused:true} from a Pausable{paused:false}. If the first Null sub-ISM in iteration order happens to be paused, the relayer packs it instead of the unpaused one, causing on-chain verification to fail even though a valid combination exists.
This only matters when: (1) threshold < sub_isms.len(), (2) non-Null successes < threshold, and (3) multiple Null sub-ISMs exist with different verify outcomes. In practice this is rare (Pausable is an emergency circuit breaker, rarely paused), and the relayer has no way to resolve it without a richer MetadataSpec. Not a code bug but a design limitation worth documenting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| run: cargo test --all-targets --features aleo,integration_test | ||
| working-directory: ./rust/main | ||
| - name: Build sealevel SBF programs | ||
| run: cargo build-sbf --manifest-path programs/ism/composite-ism/Cargo.toml |
There was a problem hiding this comment.
is this intentional? do we not build this anywhere else in CI?
I would think that in the sealevel e2e tests we would at least
fwiw I'd be comfortable without this in CI and we can try to merge #8064 instead as a path for testing this
There was a problem hiding this comment.
will review your pr regarding this
| /// `accounts_iter` is advanced for nodes that require on-chain accounts: | ||
| /// - `TrustedRelayer`: pops the relayer signer account. | ||
| /// - `Routing`: pops the domain PDA account, then may pop sub-accounts. | ||
| pub(crate) fn verify_node<'a, 'b, I>( |
There was a problem hiding this comment.
I think it'd be really nice if we were able to have an ISM that does a CPI into the Mailbox's default ISM. We do this in a bunch of places and it'd be nice to not need to express a copy of the default ISM in here. What would be the lift to do this? I assume to get the account metas things can get possibly hairy?
I think CPI-depth wise it should be okay:
Mailbox = depth 1
Custom Composite ISM = depth 2
Default ISM = depth 3
We'd want to read the default ISM from the Mailbox's Inbox PDA (instead of making an unnecessary CPI into the Mailbox to get the default ISM)
There was a problem hiding this comment.
so in evm we only fallback to the mailbox default ism in the fallback routing ism, would we do the same here? then I would probably remove the default ism from the current routing ism and introduce a new ism type FallbackRouting so we have the exact same setup as evm. Will look into this, but yeah VAM will be tricky
There was a problem hiding this comment.
Yeah I think following the behavior of DefaultFallbackRoutingIsm, but also having a normal DomainRoutingIsm behavior would be required
so maybe have the routing ISM have a configurable boolean of whether to fallback to the mailbox default ISM?
There was a problem hiding this comment.
came up with this solution now, we would have a normal RoutingIsm and a FallbackRoutingIsm, only restriction is that the fallback ism here has to also be a composite ism, else it is super difficult with the account metas
|
still reviewing but sending this off for now! |
🦀 Rust Agent Docker Image Built Successfully
Full image paths |
Scope
This PR includes the addition of a new composite ism, a few relayer changes to interact with the new ism type and composite deploy and read support in the solana rust cli.
Description
A single Solana program that stores an arbitrary ISM config tree in one PDA account and verifies messages via pure in-program recursive dispatch — no CPI calls to sub-ISM programs, no depth limits.
Why not just CPI into existing ISM programs?
Solana enforces a maximum CPI depth of 4. A real-world
Aggregation → Routing → MultisigMessageIdtree already hits the limit. More complex configs (e.g. Ethereum mainnet'sAggregation(Pausable, Routing → Aggregation(MerkleRoot, MessageId))) require depth 5+. The composite ISM avoids this entirely by keeping all verification in-program.Tree structure example (mirrors Ethereum mainnet
defaultIsm):Where to start reading
src/accounts.rs—IsmNodeenum, storage types, PDA derivation helperssrc/verify.rs— recursiveverify_nodedispatchersrc/account_metas.rs—VerifyAccountMetashandler (how the relayer discovers which accounts to pass)src/processor.rs— instruction entrypointsSupported ISM node types:
TrustedRelayerMultisigMessageIdAggregationthresholdsub-ISMs must have metadata and passRoutingmessage.originAmountRoutingRateLimitedPausableKey design decisions
MultisigMessageIdis self-contained. ECDSA verification is implemented inline rather than delegating via CPI to the existingmultisig-ism-message-idprogram. The node storesvalidators: Vec<H160>, threshold: u8directly. Domain routing is handled externally by aRoutingnode — the multisig node is domain-agnostic.Routing uses per-domain PDAs. Storing all domain→ISM mappings inline would OOM at ~100 domains (SVM heap limit is 256KB). Instead, each domain's ISM is stored in its own PDA at seeds
[b"domain_ism", &domain.to_le_bytes()]. Only the single domain PDA for the incoming message's origin is loaded at verify time — O(1) heap regardless of domain count.Two-pass
VerifyAccountMetasfor Routing. The relayer callsVerifyAccountMetas(simulation-only) to learn which accounts to include in theVerifytransaction. ForRoutingnodes, sub-accounts (e.g. aTrustedRelayer's signer pubkey) are only discoverable after reading the domain PDA — which itself must be in the account list first. The relayer loops until the account set converges:This fixpoint loop is implemented in
chains/hyperlane-sealevel/src/mailbox.rs.RateLimitedinside domain PDAs. State (filled_level,last_updated) lives inline in theIsmNode. After verification, the updated node is written back to the domain PDA if it is marked writable.VerifyAccountMetasdetectsRateLimitedin a domain PDA and marks it writable automatically.Limitations
Routingnode per deployment. Rejected atUpdateConfigtime (MultipleRoutingNodeserror).Routinginside a domain PDA.SetDomainIsmrejects trees containingRouting(RoutingInDomainIsmerror). Domain PDAs are leaves.IsmNodevariant that calls an external ISM program. The full tree must fit in one deployment.MultisigMerkleRootvariant. The MerkleRoot multisig requires a merkle proof in the verify metadata so the on-chain program can re-derive the message ID from the tree root. Hyperlane uses a fixed-depth-32 incremental Merkle tree, so the proof is always 32 × 32 = 1024 bytes. Combined with the 68-byte header, that leaves room for at most zero signatures before hitting the 1232-byte Solana packet limit — threshold=1 alone produces 1157 bytes, threshold=3 produces 1287 bytes (already over the limit before tx overhead).MultisigMessageIdsidesteps this entirely: validators sign(checkpoint, message_id)and the on-chain program recomputesmessage_id = hash(message)from the message body already present in the transaction.Solana-specific scale limits
Validated by
tests/functional_big_isms.rs, which runs the compiled BPF binary so real constraints apply (32 KB heap, 64-frame call depth, 1.4M CU budget).UpdateConfigreallocTest→Aggregation(50): works; payer must pre-fund the extra rentTransaction size is the binding constraint for deep multisig trees.
The 1232-byte Solana packet limit is a UDP/network-layer constraint — it is not enforced by
solana-program-test(neithersimulate_transactionnorprocess_transactioncheck packet size). The scale tests assert it explicitly by checking the serialized transaction length.A
3×3nested multisig tree (Agg(3)[Agg(3)[MultisigMessageId(3v,3)] ×3] ×3) produces 2463 bytes of verify metadata, which exceeds the 1232-byte limit. The ISM logic still executes correctly under BPF constraints (~1.14M CU), but theVerifytransaction cannot be submitted on mainnet as a single packet.Practical sizing guide for
MultisigMessageIdmetadata (3 sigs, 65 bytes each):Agg(3)[MultisigMessageId(3v,3) ×3]: 813 bytes — fits in one txAgg(3)[Agg(3)[MultisigMessageId(3v,3)] ×3] ×3: 2463 bytes — requires chunked deliveryTesting
tests/functional_composite.rsUpdateConfig,TransferOwnership,GetOwner,Typetests/functional_multisig.rstests/functional_aggregation.rstests/functional_routing.rstests/functional_trusted_relayer.rsis_signer=falserejection, VAM returns signer accounttests/functional_pausable.rstests/functional_rate_limited.rstests/functional_amount_routing.rstests/functional_test_ism.rstests/functional_nested.rsRoutingdomain PDA — exercises the full two-pass VAM + verify path; RateLimited state persistence across transactionstests/functional_big_isms.rs.sobinary for real BPF constraintsrun-locally/sealevel/composite_ism.rsdefaultIsmBackward compatibility
No existing on-chain deployments are affected. The
IsmNodeBorsh encoding is new — no migration needed.