feat: AdminBurnExecutor — auditable one-shot scheme for burning illegitimate G$ - #304
feat: AdminBurnExecutor — auditable one-shot scheme for burning illegitimate G$#304blueogin wants to merge 2 commits into
Conversation
… burning illegitimate tokens
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 2 other issues
Security issues:
- Detected calls to child_process from a function argument
address. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/upgrades/admin-burn-executor-deploy.ts" line_range="203-204" />
<code_context>
+ totalRefundUSD: ethers.utils.formatEther(onChainUSD),
+ owner: await executor.owner()
+ });
+ if (!onChainGD.eq(totalGD) || !onChainUSD.eq(totalUSD) || !count.eq(entries.length))
+ throw new Error("deployed executor does not match the local burn list");
+
+ // not registered yet, so canExecute is expected to be false on the scheme check
</code_context>
<issue_to_address>
**issue (broader_impact):** When `EXECUTOR` points to an existing executor, the script verifies only the aggregate G$ amount, aggregate refund amount, and entry count. A different executor with the same totals and count passes validation, so guardians can register and execute an unaudited address/amount list while the displayed burn list and resulting refund ledger refer to different accounts.
**Triggers:** When the operator uses `EXECUTOR=0x..` to skip deployment and the existing executor has different tuples but matching totals and length.
**Suggested fix:** Fetch and compare every deployed `(account, gdAmount, refundUSD)` entry against the local burn list, or require an explicit hash of the audited list.
</issue_to_address>
### Comment 2
<location path="contracts/utils/AdminBurnExecutor.sol" line_range="68" />
<code_context>
+ require(_entries.length > 0, "empty burn list");
+
+ controller = _controller;
+ token = _token;
+ owner = _owner;
+
+ uint256 gd;
</code_context>
<issue_to_address>
**issue (bug_risk):** The constructor accepts a zero `_owner`, leaving the executor permanently unable to execute or cancel because both functions require `msg.sender == owner`; if the contract is registered as a scheme, its permission cannot be removed through the executor and remains stranded until an external governance action unregisters it.
**Triggers:** When deployment is accidentally supplied with `address(0)` as `_owner`.
**Suggested fix:** Require `_owner != address(0)` in the constructor.
```suggestion
require(_owner != address(0), "owner required");
require(_entries.length > 0, "empty burn list");
```
</issue_to_address>
### Comment 3
<location path="scripts/upgrades/admin-burn-executor-deploy.ts" line_range="300" />
<code_context>
execSync(cmd, { stdio: "inherit" });
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `address`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and if the fixed list or authorization flow is wrong, the executor can irreversibly destroy listed holders' G$ balances, while the recorded USD refunds do not restore those tokens and reverting the deployment cannot undo a completed burn. The operation is bounded and requires owner execution plus guardian scheme registration, but any mistaken burn would require separate compensation or minting rather than a rerun.
Blocking findings: scripts/upgrades/admin-burn-executor-deploy.ts:204, contracts/utils/AdminBurnExecutor.sol:68, scripts/upgrades/admin-burn-executor-deploy.ts:300
| if (!onChainGD.eq(totalGD) || !onChainUSD.eq(totalUSD) || !count.eq(entries.length)) | ||
| throw new Error("deployed executor does not match the local burn list"); |
There was a problem hiding this comment.
issue (broader_impact): When EXECUTOR points to an existing executor, the script verifies only the aggregate G$ amount, aggregate refund amount, and entry count. A different executor with the same totals and count passes validation, so guardians can register and execute an unaudited address/amount list while the displayed burn list and resulting refund ledger refer to different accounts.
Triggers: When the operator uses EXECUTOR=0x.. to skip deployment and the existing executor has different tuples but matching totals and length.
Suggested fix: Fetch and compare every deployed (account, gdAmount, refundUSD) entry against the local burn list, or require an explicit hash of the audited list.
| ) { | ||
| require(address(_controller) != address(0), "controller required"); | ||
| require(address(_token) != address(0), "token required"); | ||
| require(_entries.length > 0, "empty burn list"); |
There was a problem hiding this comment.
issue (bug_risk): The constructor accepts a zero _owner, leaving the executor permanently unable to execute or cancel because both functions require msg.sender == owner; if the contract is registered as a scheme, its permission cannot be removed through the executor and remains stranded until an external governance action unregisters it.
Triggers: When deployment is accidentally supplied with address(0) as _owner.
Suggested fix: Require _owner != address(0) in the constructor.
| require(_entries.length > 0, "empty burn list"); | |
| require(_owner != address(0), "owner required"); | |
| require(_entries.length > 0, "empty burn list"); |
| const cmd = `yarn hardhat verify --contract contracts/utils/AdminBurnExecutor.sol:AdminBurnExecutor --constructor-args ${argsFile} ${address} --network ${network.name}`; | ||
| console.log("\n=== verifying ===\n" + cmd); | ||
| try { | ||
| execSync(cmd, { stdio: "inherit" }); |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument address. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
There was a problem hiding this comment.
🟡 Changes recommended
The new operational scripts include confirmed runtime-breaking issues (BigNumber underflow in supply delta reporting and incorrect fork-network detection) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an auditable, one-shot on-chain scheme (AdminBurnExecutor) and supporting operational scripts to burn illegitimate G$ balances via a new owner-only adminBurn function on SuperGoodDollar, and to output an off-chain USD refund ledger.
Changes:
- Add
adminBurn(address,uint256)toSuperGoodDollar(owner-only, works while paused, bypasses ERC777 hooks) and expose it viaISuperGoodDollar. - Add
AdminBurnExecutorscheme contract plus deploy/execute scripts (and a sample burn-list JSON) for guarded, one-time burn sweeps with an emitted refund ledger. - Add tests covering
adminBurnbehavior (authorization, paused operation, supply reduction, insufficient balance).
File summaries
| File | Description |
|---|---|
| test/token/SuperGoodDollar.test.ts | Adds unit tests for adminBurn authorization and supply/balance effects. |
| scripts/upgrades/admin-burn-list.example.json | Provides an example input format for burn/refund entries. |
| scripts/upgrades/admin-burn-executor-execute.ts | Adds an execution script to run the executor and write the refund ledger. |
| scripts/upgrades/admin-burn-executor-deploy.ts | Adds a deploy+registration proposal script for the executor. |
| contracts/utils/AdminBurnExecutor.sol | Implements one-shot scheme that burns a fixed audited list and emits refund data. |
| contracts/token/superfluid/SuperGoodDollar.sol | Adds adminBurn and removes the token recovery helper. |
| contracts/token/superfluid/ISuperGoodDollar.sol | Exposes adminBurn in the interface. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| let { name: networkName } = network; | ||
| networkName = networkName.replace("-fork", ""); | ||
|
|
||
| const isSimulation = ["hardhat", "fork", "localhost"].includes(network.name); | ||
|
|
| supplyBefore: ethers.utils.formatEther(supplyBefore), | ||
| supplyAfter: ethers.utils.formatEther(supplyAfter), | ||
| supplyDelta: ethers.utils.formatEther(supplyAfter.sub(supplyBefore)), | ||
| stillRegistered: await ctrl.isSchemeRegistered(executorAddress, release.Avatar), |
| * Flow: | ||
| * 1. deploy with the full list | ||
| * 2. guardians register this address as a scheme (genericCall permission) | ||
| * 3. anyone (or `owner`, see `execute`) calls `execute()` | ||
| * 4. the contract unregisters itself, permanently disarming it |
Description
Adds the on-chain executor and the operational scripts for running an
adminBurnsweep against a list of addresses holding illegitimate G$, plus the USD refund
ledger the sweep produces.
About # (link your issue here)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by Sourcery
Enable auditable one-shot administrative sweeps that burn illegitimate G$ and produce an off-chain USD refund ledger.
New Features:
adminBurnsupport to destroy illegitimate G$ balances, including while paused and without holder cooperation.Enhancements:
Tests: