Conversation
Code Review ReportProject: multisig-permissions (TRON Multi-Sig Skill) PR OverviewBranch Information
Commit History (multisig-relevant)
Review SummaryVerdict
Findings at a Glance
SummaryThis PR fixes several real defects in the multisig-permissions skill: a bit-ordering bug in operation encoding/decoding, a proposal-execution race condition, and missing multi-sig owner detection for One critical bug was introduced alongside the rename of Change Summary1. Bug Fix — Operation Bitmask Bit-Ordering (
|
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/utils.js |
Modified | decodeOperations and encodeOperations bit-offset formula changed from 7 - (bitIndex % 8) to bitIndex % 8 |
Purpose: Corrects TRON's LSB-first (little-endian within each byte) bit ordering convention for the operations bitmask. The previous formula inverted bits within each byte, causing wrong operation names to be decoded and wrong bitmasks to be encoded for active permissions.
2. Bug Fix — Proposal Execute Race Condition (execute.js, utils.js)
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/execute.js |
Modified | Added waitForProposalThreshold call before executing |
multisig-permissions/scripts/utils.js |
Modified | New waitForProposalThreshold function added |
Purpose: Prevents a race between approve.js writing the final signature and execute.js reading a stale proposal file. The new helper polls up to 5 times (300 ms apart) until the collected weight meets threshold before proceeding.
3. Feature — Multi-Sig Owner Detection for Permission Updates (update.js, utils.js)
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/update.js |
Modified | Detects when current owner is already multi-sig; creates pending proposal instead of broadcasting |
multisig-permissions/scripts/utils.js |
Modified | New waitForPermissionSync, permissionStateMatches, normalizePermissionShape, sameKeySet helpers |
Purpose: When the account's existing owner permission has threshold > 1, a direct broadcast would be rejected on-chain. update.js now creates a local pending proposal instead and prints the correct next step (approve.js / execute.js).
4. Feature — --account Flag for Controlled-Account Proposals (propose.js)
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/propose.js |
Modified | Added --account <target> CLI flag; renamed walletAddress → signerAddress/targetAccount |
Purpose: Allows a signer key to create a proposal on behalf of a separate controlled account (e.g., an agent key signing an active-permission transaction for a multi-sig treasury account).
5. Enhancement — Account Fetch Helper & Atomic Proposal Write (utils.js)
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/utils.js |
Modified | New getAccountInfo (fullNode REST + fallback); atomic saveProposal via tmp-file-rename |
Purpose: getAccountInfo prefers the fullNode REST endpoint (returns hex addresses; more reliable for permission data) with a fallback to tronWeb.trx.getAccount. The atomic write prevents co-signers from reading a partially-written proposal file.
6. Status Observability (status.js)
| File | Change Type | Description |
|---|---|---|
multisig-permissions/scripts/status.js |
Modified | Added operations_recognized and operations_enabled_bits fields to output |
Purpose: Gives operators visibility into the raw enabled-bit count alongside the recognized operation names, useful when custom or future operation IDs appear in the bitmask.
7. Documentation Updates (README.md, multisig-permissions/README.md, SKILL.md, USE_CASES.md)
| File | Change Type | Description |
|---|---|---|
README.md |
Modified | Added "Security & Permissions" TOC entry and skill listing |
multisig-permissions/README.md |
Modified | Documented multi-sig owner proposal flow; added key-role derivation warning |
multisig-permissions/SKILL.md |
Modified | Clarified Pattern 3 multi-sig path, env var descriptions, added safety rule #6 |
multisig-permissions/USE_CASES.md |
Modified | Corrected multi-active-id routing limitation; updated several flow descriptions |
Detailed Findings
Critical
[C-01] Stale walletAddress Reference Causes ReferenceError in propose.js
| Property | Value |
|---|---|
| Severity | Critical |
| Category | Correctness |
| File | multisig-permissions/scripts/propose.js : Lines 70–72 |
Description
The PR renames walletAddress to signerAddress throughout propose.js, but one fallback object inside main() still references the old variable name:
const perm = account.owner_permission || {
threshold: 1,
keys: [{ address: walletAddress, weight: 1 }], // ← walletAddress is undefined
};walletAddress is not declared anywhere in scope after the rename. When account.owner_permission is falsy — which can occur for newly activated TRON accounts or when the fullNode omits the field — this line throws ReferenceError: walletAddress is not defined, crashing propose.js before any transaction is built.
Code
// propose.js (lines ~70–72)
if (permissionId === 0) {
const perm = account.owner_permission || { threshold: 1, keys: [{ address: walletAddress, weight: 1 }] };
// ^^^^^^^^^^^^ ReferenceError
threshold = perm.threshold || 1;
permKeys = perm.keys || [];
}Recommendation
// Replace walletAddress with signerAddress (the new variable name)
const perm = account.owner_permission || { threshold: 1, keys: [{ address: signerAddress, weight: 1 }] };Major
[MJ-01] Silent catch {} in getAccountInfo Swallows All Errors
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Observability |
| File | multisig-permissions/scripts/utils.js : Lines 52–57 |
Description
getAccountInfo catches every exception from the fullNode REST call and discards it silently:
try {
const account = await tronWeb.fullNode.request("wallet/getaccount", { address: hexAddress, visible: false }, "post");
if (account && account.address) return account;
} catch {} // ← swallows auth errors, network failures, rate-limit errors, etc.
return tronWeb.trx.getAccount(address);This means an invalid API key, a 429 rate-limit response, or a transient network error silently falls through to the getAccount fallback. If getAccount also fails, the outer caller sees an unrelated error (or a bad account object). This makes diagnosing connectivity or auth problems very difficult in production.
Code
} catch {} // line 56Recommendation
} catch (e) {
// Log non-critical fallback; surface unexpected errors for debugging
log(`[getAccountInfo] fullNode REST failed, falling back to trx.getAccount: ${e.message}`);
}Or at minimum, re-throw for error classes that indicate misconfiguration (e.g., auth errors), and only silently fall back on 404/not-found-style responses.
[MJ-02] sameKeySet Uses Positional Array Comparison, Not Address-Based Lookup
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | multisig-permissions/scripts/utils.js : Lines 84–89 |
Description
sameKeySet compares two key arrays element-by-element at the same index:
function sameKeySet(left, right) {
if (left.length !== right.length) return false;
return left.every((item, index) =>
item.address === right[index].address && (item.weight || 1) === (right[index].weight || 1)
);
}This is used by permissionStateMatches (called from waitForPermissionSync) to confirm that a permission update has propagated to the node. If the TRON node returns keys in a different order than the order they were submitted in — which is implementation-dependent and not guaranteed — sameKeySet will return false even though the actual key set is identical. This will cause waitForPermissionSync to always time out and report sync_warning, even for successful updates.
The same positional comparison is also applied to active permissions in permissionStateMatches.
Code
// utils.js – sameKeySet
return left.every((item, index) =>
item.address === right[index].address && ... // position-sensitive
);Recommendation
// Sort both arrays by address before comparing, or use Set-based lookup:
function sameKeySet(left, right) {
if (left.length !== right.length) return false;
const rightMap = new Map(right.map(k => [k.address, k.weight || 1]));
return left.every(item => rightMap.get(item.address) === (item.weight || 1));
}For permissionStateMatches, compare active permissions by id rather than by array index (the id field is already compared inside the every callback, but using index to pair them is still order-dependent).
[MJ-03] multiSign Called with undefined Private Key — Undocumented Behaviour
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Security |
| File | multisig-permissions/scripts/update.js : Line ~335 |
Description
When multi-sig owner signing is required, update.js calls:
const signed = requiresOwnerMultisig
? await tronWeb.trx.multiSign(tx, undefined, 0)
: await tronWeb.trx.sign(tx);Passing undefined as the private key to multiSign relies on TronWeb resolving the key from tronWeb.defaultAddress.privateKey internally. This is not part of TronWeb's public API contract. If TronWeb's behaviour changes, or if the private key is not set in the TronWeb instance (e.g., a future refactor passes in a read-only instance by mistake), this will either sign with the wrong key or throw an opaque error. The explicit key should be passed instead.
Code
await tronWeb.trx.multiSign(tx, undefined, 0) // undefined = rely on internal defaultRecommendation
const privateKey = process.env.TRON_PRIVATE_KEY;
const signed = requiresOwnerMultisig
? await tronWeb.trx.multiSign(tx, privateKey, 0)
: await tronWeb.trx.sign(tx, privateKey);This matches the explicit pattern used by other scripts and removes the dependency on undocumented TronWeb internals.
Minor
[MN-01] waitForProposalThreshold Returns Without Error When Threshold Not Met
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | multisig-permissions/scripts/utils.js : Lines 332–348 |
Description
After exhausting all polling attempts, waitForProposalThreshold returns the last-loaded proposal silently, even if the threshold was never met:
for (let i = 0; i < attempts; i++) {
if (collectedWeight >= proposal.threshold) return proposal;
...
proposal = loadProposal(idOrPath);
}
return proposal; // ← returns regardless of whether threshold was metexecute.js does perform its own threshold check after calling waitForProposalThreshold, so the execution path correctly guards against broadcasting an under-signed transaction. However, the polling function silently returning an under-threshold result is misleading — callers cannot distinguish "threshold met" from "still short" without re-doing the weight arithmetic. A returned status field or a distinct sentinel would make this clearer.
Recommendation
Return a result object that carries the threshold-met state:
return { proposal, thresholdMet: false };
// or: return { proposal, collectedWeight, thresholdMet: collectedWeight >= proposal.threshold };And update execute.js to destructure accordingly (its own guard already handles the case).
[MN-02] --account Flag Value Is Not Validated as a TRON Address
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | multisig-permissions/scripts/propose.js : Lines 43, 56 |
Description
The new --account flag value is accepted verbatim and passed directly to getAccountInfo and transactionBuilder calls without any format validation:
if (args[i] === "--account" && args[i + 1]) { flags.account = args[++i]; continue; }
...
const targetAccount = opts.account || signerAddress;An invalid address (e.g., a typo, an Ethereum-style 0x address, or a flag value accidentally passed as the account) would fail with a cryptic TronWeb error deep in the call stack rather than an early, user-friendly validation message.
Recommendation
// After parsing, validate targetAccount is a valid Base58 TRON address:
if (opts.account && !tronWeb.isAddress(opts.account)) {
outputJSON({ error: `--account value is not a valid TRON address: "${opts.account}"` });
process.exit(1);
}Suggestions
[S-01] Bit-Ordering Fix Deserves an Explanatory Comment
File: multisig-permissions/scripts/utils.js — decodeOperations / encodeOperations
Description: The change from 7 - (bitIndex % 8) to bitIndex % 8 is a meaningful correctness fix but is completely uncommented. TRON's operation bitmask uses LSB-first ordering within each byte, which is counter-intuitive. Future contributors may "fix" this back to MSB-first if there's no comment explaining the convention.
Suggestion:
// TRON operation bitmask uses LSB-first ordering within each byte:
// bit 0 of byte 0 = operation 0, bit 1 of byte 0 = operation 1, etc.
const bitOffset = bitIndex % 8; // LSB-first: do NOT use (7 - bitIndex % 8)[S-02] saveProposal Atomic Write Could Include a Short Comment
File: multisig-permissions/scripts/utils.js — saveProposal
Description: The tmp-file-then-rename pattern is correct and a genuine improvement (prevents co-signers from reading a partially written JSON). A one-line comment explaining why the rename pattern is used would help future maintainers understand it is intentional, not over-engineering.
Suggestion:
// Write to a temp file then rename atomically to prevent co-signers
// from reading a partially written proposal file.
const tmpPath = path.join(dir, `.${proposal.proposalId}.${process.pid}.${Date.now()}.tmp`);[S-03] classifySecurity "strong" Upgrade Logic May Be Too Permissive for Unrecognized Operations
File: multisig-permissions/scripts/utils.js — classifySecurity Lines ~280–285
Description: The upgraded "strong" classification check uses:
return enabledBits > 0 && (ops.length > 0 || enabledBits === 1);The enabledBits === 1 branch allows a permission with exactly one unrecognized operation bit to count as "scoped" and potentially contribute to a "strong" rating. If an operator has a custom operation ID that this skill doesn't know about, they could unknowingly have a permission rated "strong" that grants a broader-than-expected scope. Consider requiring ops.length > 0 (at least one recognized operation) for the "scoped" determination.
Positive Observations
| Area | Observation |
|---|---|
| Race condition fix | The waitForProposalThreshold polling in execute.js is a clean, targeted fix for a real concurrency hazard in the co-signing flow. |
| Atomic proposal write | Using tmp-file + renameSync in saveProposal is the correct pattern for file-system atomicity and shows defensive programming. |
| Multi-sig owner detection | update.js correctly gates on currentOwnerPerm.threshold > 1 before signing, ensuring the right signing path is taken based on live on-chain state. |
| Bit-ordering correctness | The 7 - (bitIndex % 8) → bitIndex % 8 fix is correct per TRON protocol and affects the accuracy of all operation encoding/decoding. |
| Observability additions | operations_enabled_bits alongside operations_recognized in status.js output is valuable for diagnosing permission configs with custom or future operation codes. |
| Documentation accuracy | Correcting USE_CASES.md to honestly state that multi-active-id routing is not yet implemented (rather than describing it as working) prevents operator confusion and potential misuse. |
| Key-role derivation warnings | The repeated warnings in README, SKILL.md, and docs that variable names are "only labels" and the signer role is determined by the derived address are important security guidance for this kind of permission system. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 5 | 3 | 0 | Stale walletAddress, positional key comparison, multiSign(undefined) |
| Security | 6 | 5 | 1 | 0 | Silent catch swallows auth errors; no input validation on --account |
| Performance | 4 | 4 | 0 | 0 | Polling loops are bounded; no N+1 issues introduced |
| Code Quality | 6 | 5 | 1 | 0 | Missing comment on bit-ordering fix and atomic write |
| Testing | 3 | 0 | 0 | 3 | No test files changed or added in diff |
| Documentation | 5 | 5 | 0 | 0 | Docs meaningfully improved; corrections are accurate |
| Compatibility | 3 | 3 | 0 | 0 | No breaking API changes; fallback path maintained in getAccountInfo |
| Observability | 3 | 3 | 0 | 0 | New bit-count field in status output; sync warning on timeout |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between origin/main and origin/dev/pr2-multisig-fixes. Runtime behavior, integration testing against a live TRON node, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-27
3 participants
No description provided.