Verify Wormhole VAA fields properly and bound governance proposal length - #469
Merged
abayomicornelius merged 4 commits intoAug 24, 2026
Conversation
complete_bridge_transfer's trusted-emitter lookup keyed off transfer.source_chain — a field decoded from the payload bytes themselves, which parse_bridge_payload doesn't independently validate — instead of parsed.emitter_chain, the chain ID Wormhole's guardian network actually cryptographically attests as the VAA's real origin. Using a sender-supplied payload field for a trust decision instead of the VAA-verified envelope metadata is exactly backwards for a security check: if payload content and VAA envelope ever diverge (e.g. a misconfigured or malicious source-chain contract embedding the wrong chain ID in its own outbound payload), the allowlist lookup would key off the wrong value. The BridgeTransferCompleted event had the same issue, reporting the unverified field. Use parsed.emitter_chain for both the TrustedEmitter lookup and the completion event. Closes Heliobond#452
…ge_transfer BridgeTransferPayload.token_address exists specifically to identify which asset a cross-chain message concerns, and initiate_bridge_transfer sets it to the vault's own encoded address on the outbound side. But complete_bridge_transfer decoded the full payload and never once read transfer.token_address — it minted HBS purely from the trusted-emitter check plus recipient/amount, with no assertion the incoming message was actually about this vault's own token. Not exploitable in isolation today since the trusted-(chain, emitter) allowlist already gates who can trigger a mint, but if the same trusted emitter is ever reused for a multi-asset bridge (a common pattern) or a future version adds more token types, a VAA meant for a different asset would be silently accepted and minted as HBS. Assert transfer.token_address matches this vault's own contract address before minting. New VaultError::BridgeTokenMismatch. Closes Heliobond#453
…e_transfer BridgeTransferPayload.target_chain is set by the sender on the outbound side to indicate which chain a transfer is destined for. Same gap as the token_address issue: complete_bridge_transfer decoded this field into transfer.target_chain but never read it anywhere, relying entirely on the trusted-emitter allowlist to prevent misuse rather than also checking the message actually claims Stellar as its destination. Assert transfer.target_chain == wormhole::chain_id::STELLAR before proceeding to mint. New VaultError::BridgeWrongTargetChain. Closes Heliobond#454
create_project's uri is strictly bounded (MIN_URI_LEN/MAX_URI_LEN) explicitly to prevent excessively large ledger entries, but create_proposal's description had no equivalent bound — any whitelisted address (not just the owner) could call create_proposal with an arbitrarily large description, stored indefinitely as part of the Proposal persistent entry. Distinct from Heliobond#332, which covers compact_storage's unbounded Vec params and the missing voting_duration_secs upper bound, not description's length. Add MAX_PROPOSAL_DESCRIPTION_LEN (2048 bytes), mirroring the URI-length pattern, with a new RegistryError::ProposalDescriptionTooLong variant. Closes Heliobond#455
|
@Temi-suwa18 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
2 participants
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.
Summary
Four independent fixes: three tighten
complete_bridge_transfer's validation of an inbound Wormhole VAA, one bounds an unbounded string field inproject_registry.Trust decision used unverified payload field instead of VAA-verified one (#452)
The trusted-emitter lookup keyed off
transfer.source_chain— decoded from the payload bytes themselves, whichparse_bridge_payloaddoesn't independently validate — instead ofparsed.emitter_chain, the chain ID Wormhole's guardian network actually cryptographically attests as the VAA's real origin. Switched both theTrustedEmitterlookup and theBridgeTransferCompletedevent to useparsed.emitter_chain.token_addressdecoded but never checked (#453)BridgeTransferPayload.token_addressidentifies which asset a message concerns, butcomplete_bridge_transferdecoded it and never read it — minting was gated purely by the trusted-emitter check plusrecipient/amount. Not exploitable in isolation today, but if the same trusted emitter is ever reused for a multi-asset bridge, a VAA about a different asset would be silently minted as HBS. Added an assertion thattransfer.token_addressmatches this vault's own contract address.target_chaindecoded but never checked (#454)Same pattern:
transfer.target_chainis meant to confirm a message is actually destined for Stellar, but was decoded and ignored. Added an assertion thattransfer.target_chain == wormhole::chain_id::STELLAR.create_proposal's description has no length bound (#455)create_project'suriis strictly bounded (MIN_URI_LEN/MAX_URI_LEN) specifically to prevent excessively large ledger entries, butcreate_proposal'sdescriptionhad no equivalent bound — any whitelisted address could store an arbitrarily large description indefinitely. AddedMAX_PROPOSAL_DESCRIPTION_LEN(2048 bytes), mirroring the URI pattern. Distinct from #332 (unboundedVecparams / missingvoting_duration_secsupper bound).Why these are safe, minimal fixes
All four are additional validation checks on an already-gated entry point (the trusted-emitter allowlist already governs who can trigger a mint at all) or a straightforward length bound matching an existing pattern in the same file — no changes to any existing authorized behavior's happy path, no storage layout changes.
stellar contract buildpasses clean across the whole workspace.Closes
Closes #452
Closes #453
Closes #454
Closes #455
Test plan
stellar contract build(full workspace, wasm32v1-none) — passes clean