Repository navigation
feat(sdk-coin-xlm): report full setOptions fields and unmodelled ops in explain #10095
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,25 +127,70 @@ interface TransactionMemo { | |
| type?: string; | ||
| } | ||
|
|
||
| /** | ||
| * A signer added, re-weighted or (weight 0) removed by a setOptions operation. | ||
| * ed25519PublicKey and ed25519SignedPayload keys are strkeys; sha256Hash and preAuthTx keys are hex. | ||
| */ | ||
| interface TransactionSigner { | ||
| type: 'ed25519PublicKey' | 'sha256Hash' | 'preAuthTx' | 'ed25519SignedPayload'; | ||
| key: string; | ||
| weight?: number; | ||
| } | ||
|
|
||
| interface TransactionOperation { | ||
| // 'unsupported' marks an operation explain does not model, so it is never silently absent from the result | ||
| type: string; | ||
| coin: string; | ||
| // set only when the operation names its own source account, i.e. it runs against an account other than the tx source | ||
| source?: string; | ||
| limit?: string; | ||
| asset?: stellar.Asset; | ||
| // unsupported operation field: the stellar operation type that was not modelled | ||
| operationType?: string; | ||
| // accountConfig (setOptions) operation fields; each is undefined when the operation does not set it | ||
| setFlags?: number; | ||
| clearFlags?: number; | ||
| masterWeight?: number; | ||
| lowThreshold?: number; | ||
| medThreshold?: number; | ||
| highThreshold?: number; | ||
| homeDomain?: string; | ||
| inflationDest?: string; | ||
| signer?: TransactionSigner; | ||
| // authorizeTrustline (setTrustLineFlags) operation fields | ||
| trustor?: string; | ||
| // Each trustline flag is true when the operation sets it, false when it clears it, and undefined when the | ||
| // operation does not touch it. undefined is therefore never a revoke, and an operation can deauthorize a | ||
| // trustline through authorizedToMaintainLiabilities or authorized, so consumers must read all three flags. | ||
| authorized?: boolean; | ||
| authorizedToMaintainLiabilities?: boolean; | ||
| clawbackEnabled?: boolean; | ||
| // clawback operation fields | ||
| from?: string; | ||
| amount?: string; | ||
| // clawback asset, so consumers need not parse the coin name to label the amount | ||
| assetCode?: string; | ||
| assetIssuer?: string; | ||
| } | ||
|
|
||
| function describeSigner(signer: stellar.Signer): TransactionSigner { | ||
| const weight = signer.weight === undefined ? undefined : Number(signer.weight); | ||
| if ('ed25519PublicKey' in signer) { | ||
| return { type: 'ed25519PublicKey', key: signer.ed25519PublicKey, weight }; | ||
| } | ||
| if ('sha256Hash' in signer) { | ||
| return { type: 'sha256Hash', key: signer.sha256Hash.toString('hex'), weight }; | ||
| } | ||
| if ('preAuthTx' in signer) { | ||
| return { type: 'preAuthTx', key: signer.preAuthTx.toString('hex'), weight }; | ||
| } | ||
| return { type: 'ed25519SignedPayload', key: signer.ed25519SignedPayload, weight }; | ||
| } | ||
|
|
||
| interface TransactionOutput extends BaseTransactionOutput { | ||
| coin: string; | ||
| // set only when the operation names its own source account, i.e. it runs against an account other than the tx source | ||
| source?: string; | ||
| } | ||
|
|
||
| interface TransactionExplanation extends BaseTransactionExplanation { | ||
|
|
@@ -1052,6 +1097,7 @@ export class Xlm extends BaseCoin { | |
| ), | ||
| address: op.destination + memoId, | ||
| coin, | ||
| source: op.source, | ||
| }; | ||
|
|
||
| if (!_.isUndefined(spendAmounts[coin])) { | ||
|
|
@@ -1072,6 +1118,7 @@ export class Xlm extends BaseCoin { | |
| operations.push({ | ||
| type: op.type, | ||
| coin: this.getTokenNameFromStellarAsset(asset), | ||
| source: op.source, | ||
| asset, | ||
| limit: this.bigUnitsToBaseUnits(op.limit), | ||
| }); | ||
|
|
@@ -1080,26 +1127,41 @@ export class Xlm extends BaseCoin { | |
| operations.push({ | ||
| type: 'accountConfig', | ||
| coin: this.getChain(), | ||
| source: op.source, | ||
| setFlags: setOptionsOp.setFlags, | ||
| clearFlags: setOptionsOp.clearFlags, | ||
| masterWeight: setOptionsOp.masterWeight, | ||
| lowThreshold: setOptionsOp.lowThreshold, | ||
| medThreshold: setOptionsOp.medThreshold, | ||
| highThreshold: setOptionsOp.highThreshold, | ||
| homeDomain: setOptionsOp.homeDomain, | ||
| inflationDest: setOptionsOp.inflationDest, | ||
| signer: setOptionsOp.signer ? describeSigner(setOptionsOp.signer) : undefined, | ||
| }); | ||
| } else if (op.type === 'setTrustLineFlags') { | ||
| const setTrustLineFlagsOp = op as stellar.Operation.SetTrustLineFlags; | ||
|
Comment on lines
1129
to
1142
There was a problem hiding this comment. 🟣 pre-existing, not blocking: pre-existing, security-relevant: Reviewers cannot tell when any operation runs against a different account, because no explained operation reports its op-level source. The explain loop at xlm.ts:1047-1124 ignores op.source for setOptions, setTrustLineFlags, clawback, changeTrust, payment and createAccount. A setOptions with source set to another wallet looks the same as one for the tx source. Fix: add op.source (undefined when unset) to every operations entry and output the loop builds, including the new unsupported entries. The verify path already rejects source on setOptions. Why this was flaggedTrigger: a transaction whose setOptions or clawback op carries source: otherAccount, for example stellar.Operation.setOptions({ source: victim, masterWeight: 0 }), is passed to explainTransaction. Each branch of the forEach at xlm.ts:1047-1125 builds its entry only from op-specific fields, and none reads op.source. The explanation therefore shows masterWeight: 0 as if it applied to the transaction's own account. verifyAccountConfigOperations (xlm.ts:1263-1281) names 'source' as dangerous, saying an op redirected to another account runs under that account's thresholds. That check only runs on the verify path and only for setOptions. The base omits source in the same way. This PR's stated aim is that a reviewer sees everything a transaction does, and it adds fields to these same entries without this one. Verification: The trigger is a txBase64 whose op carries an op-level
Contributor
Author
There was a problem hiding this comment. Addressed in 416b1f5. Every |
||
| operations.push({ | ||
| type: 'authorizeTrustline', | ||
| coin: this.getTokenNameFromStellarAsset(setTrustLineFlagsOp.asset), | ||
| source: op.source, | ||
| trustor: setTrustLineFlagsOp.trustor, | ||
| authorized: setTrustLineFlagsOp.flags?.authorized, | ||
| authorizedToMaintainLiabilities: setTrustLineFlagsOp.flags?.authorizedToMaintainLiabilities, | ||
| clawbackEnabled: setTrustLineFlagsOp.flags?.clawbackEnabled, | ||
| }); | ||
| } else if (op.type === 'clawback') { | ||
| const clawbackOp = op as stellar.Operation.Clawback; | ||
| operations.push({ | ||
| type: 'clawback', | ||
| coin: this.getTokenNameFromStellarAsset(clawbackOp.asset), | ||
| source: op.source, | ||
| from: clawbackOp.from, | ||
| amount: this.bigUnitsToBaseUnits(clawbackOp.amount), | ||
| assetCode: clawbackOp.asset.code, | ||
| assetIssuer: clawbackOp.asset.issuer, | ||
| }); | ||
| } else { | ||
| operations.push({ type: 'unsupported', coin: this.getChain(), operationType: op.type, source: op.source }); | ||
| } | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 pre-existing, not blocking: pre-existing, security-relevant: A reviewer still sees no sign of a setOptions that adds a signer, which is the most dangerous account change. The accountConfig entry at xlm.ts:1093-1104 omits setOptionsOp.signer. A signer-only op shows up as accountConfig with every field undefined, which looks like a no-op. Fix: report signer (key and weight, with weight 0 meaning removal) on accountConfig entries. The PR says this omission is intentional, but the stated goal is that no signed account change is hidden.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: a txBase64 holding stellar.Operation.setOptions({ signer: { ed25519PublicKey: attacker, weight: 3 } }) is passed to explainTransaction. The setOptions branch at xlm.ts:1091-1104 copies setFlags, clearFlags, masterWeight, the three thresholds, homeDomain and inflationDest, but never signer. The result is { type: 'accountConfig', coin } with every other field undefined. verifyAccountConfigOperations (xlm.ts:1269-1281) lists 'signer' among the fields that matter, citing signer:{attacker,weight:3}, but that check only runs on the verify path, not for explain consumers such as offline vault review. The base behaves the same way. Still, this PR extends the setOptions output so that a reviewer can see every account change, and it leaves out the field that hands over control of the account. The PR description calls the omission intentional but gives no mitigation for explain readers.
Verification: Trigger: a setOptions op that sets only
signeris passed to explainTransaction. The setOptions branch at modules/sdk-coin-xlm/src/xlm.ts:1091-1104 never readssetOptionsOp.signer. So a signer-only op comes out as{ type: 'accountConfig', coin }, which looks like a no-op. The 'signer' guard in verifyAccountConfigOperations only runs in verifyTransaction. The base had the same omission.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 416b1f5.
accountConfigentries now carrysigneras{ type, key, weight }(ed25519 public key, sha256 hash, pre-auth tx and signed payload signers). Weight0is kept and means removal, so a signer-onlysetOptionsno longer reads as an empty entry. Tests cover add, removal, hex-encoded hash and pre-auth signers, and the unset case.