Handle muxed account during buildTransaction and CheckForForbiddenSigners#500
Handle muxed account during buildTransaction and CheckForForbiddenSigners#500aditya1702 merged 3 commits intomainfrom
buildTransaction and CheckForForbiddenSigners#500Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a security gap where muxed (M...) Stellar addresses could bypass forbidden-signer/channel-account source checks by resolving them to their underlying Ed25519 G-address before validation.
Changes:
- Added
ResolveToGAddressutility to normalize G/M addresses to the underlying G-address. - Updated
CheckForForbiddenSignersto validate muxed op source accounts against forbidden signers. - Updated transaction building validation to block using the channel account via muxed source accounts; added corresponding tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/utils.go | Adds ResolveToGAddress to normalize muxed addresses for consistent validation. |
| pkg/utils/utils_test.go | Adds unit tests for ResolveToGAddress (empty, G, M, invalid). |
| pkg/sorobanauth/sorobanauth.go | Resolves op source to G-address before forbidden-signer checks. |
| pkg/sorobanauth/sorobanauth_test.go | Adds muxed-account cases to forbidden-signer tests. |
| internal/services/transaction_service.go | Resolves op source to G-address to prevent channel-account bypass via muxed addresses. |
| internal/services/transaction_service_test.go | Adds regression test ensuring muxed channel-account sources are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ResolveToGAddress resolves a Stellar address (G-address or M-address) to its underlying G-address. | ||
| // For G-addresses, it returns the address unchanged. For M-addresses (SEP-23), it strips the memo ID. | ||
| func ResolveToGAddress(address string) (string, error) { | ||
| if address == "" { | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
The doc comment for ResolveToGAddress doesn’t mention that an empty string is treated as a valid input and returns "" with a nil error. Consider documenting this behavior (or returning an error) so callers don’t accidentally treat missing source accounts as successfully-resolved addresses. Also, the muxed account’s 64-bit value is an ID; calling it a “memo ID” here may be misleading—“muxed account ID” would be clearer.
2 participants
What
Why
The current logic incorrectly bypasses our checks if an address is a muxed account with the wallet-backend's channel account as the G address. This is a security bug and we now correctly handle it.
Known limitations
N/A
Issue that this PR addresses
Closes #499