feat: AdminBurnExecutor — auditable one-shot scheme for burning illegitimate G$ - #305
feat: AdminBurnExecutor — auditable one-shot scheme for burning illegitimate G$#305blueogin wants to merge 2 commits into
Conversation
![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="128-141" />
<code_context>
+ throw new Error(`token owner ${owner} is not the Avatar ${release.Avatar} - adminBurn would revert`);
+
+ // the executor is useless against an implementation that has no adminBurn
+ if (!supergd.interface.functions["adminBurn(address,uint256)"])
+ throw new Error("local artifacts have no adminBurn(address,uint256) - run `yarn compile`");
+ // ...and against a *live* implementation that has no adminBurn either. The
+ // proxy would delegate the call into an implementation that reverts, so check
</code_context>
<issue_to_address>
**issue (bug_risk):** The deployment script always throws because the diff does not add `adminBurn(address,uint256)` to the `SuperGoodDollar` artifact or implementation. `supergd.interface.functions[...]` is therefore undefined, so no executor can be deployed through this script.
**Triggers:** When this PR is compiled against the repository as shown.
**Suggested fix:** Add the `adminBurn` implementation and update the deployed implementation before relying on this script, or remove this check until that token change is included.
```suggestion
```
</issue_to_address>
### Comment 2
<location path="scripts/upgrades/admin-burn-executor-deploy.ts" line_range="205-206" />
<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$ total, refund total, and entry count. A different fixed list with the same aggregates passes validation, so the script can preflight and execute an unaudited set of accounts or amounts while presenting the local list as the target.
**Triggers:** When an existing executor has the same entry count and aggregate totals as the local burn list but different tuples.
**Suggested fix:** Compare every on-chain `(account, gdAmount, refundUSD)` entry against the locally loaded list in order, and reject any mismatch.
</issue_to_address>
### Comment 3
<location path="scripts/upgrades/admin-burn-executor-deploy.ts" line_range="302" />
<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 once the scheme is registered and executed, the fixed list can permanently burn token balances through the Controller's genericCall authority, and an incorrect list cannot be undone by reverting the deployment. The recorded refund ledger can also create off-chain compensation obligations, while the authorization and target list are themselves the irreversible decision.
Blocking findings: scripts/upgrades/admin-burn-executor-deploy.ts:141, scripts/upgrades/admin-burn-executor-deploy.ts:206, scripts/upgrades/admin-burn-executor-deploy.ts:302
| // the executor is useless against an implementation that has no adminBurn | ||
| if (!supergd.interface.functions["adminBurn(address,uint256)"]) | ||
| throw new Error("local artifacts have no adminBurn(address,uint256) - run `yarn compile`"); | ||
| // ...and against a *live* implementation that has no adminBurn either. The | ||
| // proxy would delegate the call into an implementation that reverts, so check | ||
| // the selector is actually present in the deployed code. | ||
| const selector = supergd.interface.getSighash("adminBurn(address,uint256)"); | ||
| const liveImpl = await supergd.getCodeAddress(); | ||
| const liveCode = await ethers.provider.getCode(liveImpl); | ||
| if (!liveCode.includes(selector.slice(2)) && !process.env.SKIP_IMPL_CHECK) | ||
| throw new Error( | ||
| `live SuperGoodDollar implementation ${liveImpl} has no adminBurn - run supergooddollar-admin-burn.ts first` | ||
| ); | ||
| console.log("live implementation:", liveImpl, "(adminBurn present)"); |
There was a problem hiding this comment.
issue (bug_risk): The deployment script always throws because the diff does not add adminBurn(address,uint256) to the SuperGoodDollar artifact or implementation. supergd.interface.functions[...] is therefore undefined, so no executor can be deployed through this script.
Triggers: When this PR is compiled against the repository as shown.
Suggested fix: Add the adminBurn implementation and update the deployed implementation before relying on this script, or remove this check until that token change is included.
| // the executor is useless against an implementation that has no adminBurn | |
| if (!supergd.interface.functions["adminBurn(address,uint256)"]) | |
| throw new Error("local artifacts have no adminBurn(address,uint256) - run `yarn compile`"); | |
| // ...and against a *live* implementation that has no adminBurn either. The | |
| // proxy would delegate the call into an implementation that reverts, so check | |
| // the selector is actually present in the deployed code. | |
| const selector = supergd.interface.getSighash("adminBurn(address,uint256)"); | |
| const liveImpl = await supergd.getCodeAddress(); | |
| const liveCode = await ethers.provider.getCode(liveImpl); | |
| if (!liveCode.includes(selector.slice(2)) && !process.env.SKIP_IMPL_CHECK) | |
| throw new Error( | |
| `live SuperGoodDollar implementation ${liveImpl} has no adminBurn - run supergooddollar-admin-burn.ts first` | |
| ); | |
| console.log("live implementation:", liveImpl, "(adminBurn present)"); |
| 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$ total, refund total, and entry count. A different fixed list with the same aggregates passes validation, so the script can preflight and execute an unaudited set of accounts or amounts while presenting the local list as the target.
Triggers: When an existing executor has the same entry count and aggregate totals as the local burn list but different tuples.
Suggested fix: Compare every on-chain (account, gdAmount, refundUSD) entry against the locally loaded list in order, and reject any mismatch.
| 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
There are confirmed correctness issues in the operational scripts (BigNumber underflow in the ledger summary and fork-network simulation detection), plus missing automated test coverage for a high-privilege burn scheme.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an on-chain, fixed-list “AdminBurnExecutor” scheme plus operational Hardhat scripts to deploy, register (via guardians Safe), and execute an auditable one-shot adminBurn sweep while emitting/writing a per-account USD refund ledger.
Changes:
- Introduces
contracts/utils/AdminBurnExecutor.sol, a one-time DAOStack scheme that burns a constructor-fixed list and self-unregisters (or can be cancelled). - Adds deployment tooling to validate a burn list, deploy/verify an executor, and propose
Controller.registerScheme(..., genericCall)via Safe. - Adds an execution script to preflight, execute (or cancel), and output a JSON refund ledger.
File summaries
| File | Description |
|---|---|
| scripts/upgrades/admin-burn-list.example.json | Example burn-list input format (human units + wei variants). |
| scripts/upgrades/admin-burn-executor-deploy.ts | Validates list, deploys/verifies executor, proposes scheme registration via Safe/guardian simulation. |
| scripts/upgrades/admin-burn-executor-execute.ts | Preflights and executes/cancels the executor, then generates a refund ledger from emitted events. |
| contracts/utils/AdminBurnExecutor.sol | New fixed-list, one-shot burn scheme that uses genericCall to invoke adminBurn and then unregisters itself. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| * All or nothing: if a single burn fails, the whole transaction reverts and | ||
| * the scheme stays armed so it can be retried. | ||
| */ | ||
| function execute() external { |
There was a problem hiding this comment.
@blueogin should try every address. emit event for failed addresses.
| let { name: networkName } = network; | ||
| networkName = networkName.replace("-fork", ""); | ||
|
|
||
| const isSimulation = ["hardhat", "fork", "localhost"].includes(network.name); |
| totalRefundUSD: ethers.utils.formatEther(totalUSD), | ||
| supplyBefore: ethers.utils.formatEther(supplyBefore), | ||
| supplyAfter: ethers.utils.formatEther(supplyAfter), | ||
| supplyDelta: ethers.utils.formatEther(supplyAfter.sub(supplyBefore)), |
| * 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()` |
| * Guardians can call this before signing the scheme registration, and it is | ||
| * re-checked inside `execute`. |
| * All or nothing: if a single burn fails, the whole transaction reverts and | ||
| * the scheme stays armed so it can be retried. | ||
| */ | ||
| function execute() external { |
There was a problem hiding this comment.
@blueogin should try every address. emit event for failed addresses.
…roved error handling
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
Introduce an auditable AdminBurnExecutor workflow for sweeping illegitimate G$ and generating the corresponding off-chain USD refund ledger.
New Features:
Enhancements:
Deployment:
Chores: