feat: block transfers to/from known pools in SuperGoodDollar - #306
feat: block transfers to/from known pools in SuperGoodDollar#306blueogin wants to merge 3 commits into
Conversation
Adds an owner-managed blocklist to the token: blocked addresses (eg. known DEX pools) can neither send nor receive G$. The check lives in _processFees, the single choke point for ERC20/ERC677/ERC777 transfers and the superfluid host batch operations. Superfluid streams settle via the agreement layer and are not covered - a stream can still credit a blocked address, but it can not move funds out. Adding the blocklist pushed the contract over the 24576 byte code size limit (optimizer is already at runs: 0), so four revert strings were converted to custom errors to make room. Final size is 24538 bytes. Also adds scripts/upgrades/supergooddollar-block-pools.ts, which deploys the new implementation and proposes updateCode + setBlocked per pool. The pool address list is intentionally empty and must be filled in before running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/upgrades/supergooddollar-block-pools.ts" line_range="129" />
<code_context>
+ }
+};
+
+export const main = async () => {
+ await upgrade().catch(console.log);
+};
+
+if (process.argv[1].includes("supergooddollar-block-pools")) main();
</code_context>
<issue_to_address>
**issue (bug_risk):** The script catches every upgrade, proposal, and verification failure with `console.log` and does not rethrow or set a failing exit status, so CI or an operator invoking the script receives a successful process result even when the upgrade was not executed.
**Triggers:** When deployment, governance proposal creation, or post-execution verification fails.
**Suggested fix:** Log the error and rethrow it, or set `process.exitCode = 1` in the catch handler.
```suggestion
await upgrade().catch(error => { console.log(error); process.exitCode = 1; });
```
</issue_to_address>
### Comment 2
<location path="contracts/token/superfluid/SuperGoodDollar.sol" line_range="379" />
<code_context>
emit IERC20.Transfer(account, address(0), amount);
}
+ /**
+ * @dev Blocks/unblocks accounts (eg. known liquidity pools) from sending or receiving G$.
+ * Owner only.
</code_context>
<issue_to_address>
**nitpick:** The `_processFees` documentation says it is called by every G$ movement, but minting, burning, administrative burns, and Superfluid agreement settlement do not call it; consequently those paths do not enforce the blocklist.
**Suggested fix:** Describe the exact covered transfer surfaces and explicitly exclude mint, burn, administrative burn, and agreement settlement paths.
```suggestion
* Called by ERC20, ERC677, and ERC777 transfers and Superfluid host batch operations; not by minting, burning, administrative burns, or Superfluid agreement settlement.
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the upgrade adds an owner-controlled transfer restriction, so a mistakenly configured pool can no longer send or receive G$ and live trades may fail. Unblocking the address or reverting the implementation restores future transfers, but transactions and service disruption that occurred while it was blocked cannot be undone.
Blocking findings: scripts/upgrades/supergooddollar-block-pools.ts:129
| /** | ||
| * @dev Sends transactional fees to feeRecipient address from given address | ||
| * @dev Enforces the blocklist and sends transactional fees to feeRecipient address from given address. | ||
| * Called by every G$ movement (ERC20/ERC677/ERC777 and the superfluid host batch operations). |
There was a problem hiding this comment.
nitpick: The _processFees documentation says it is called by every G$ movement, but minting, burning, administrative burns, and Superfluid agreement settlement do not call it; consequently those paths do not enforce the blocklist.
Suggested fix: Describe the exact covered transfer surfaces and explicitly exclude mint, burn, administrative burn, and agreement settlement paths.
| * Called by every G$ movement (ERC20/ERC677/ERC777 and the superfluid host batch operations). | |
| * Called by ERC20, ERC677, and ERC777 transfers and Superfluid host batch operations; not by minting, burning, administrative burns, or Superfluid agreement settlement. |
There was a problem hiding this comment.
🟡 Changes recommended
The new upgrade script contains a misleading network usage hint and a verified no-op settings merge (defaultsDeep(...) return value ignored) that should be corrected before relying on it operationally.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an owner-managed transfer blocklist to SuperGoodDollar to prevent transfers to/from known pool addresses (and any other configured accounts), and introduces a governance/upgrade script to deploy the new implementation and block an initial set of pools in a single proposal.
Changes:
- Add
isBlockedmapping +setBlocked()(owner-only) and enforce blocking at the_processFeeschoke point. - Replace several revert strings with custom errors (including fee-balance failures) and update tests accordingly.
- Add an upgrade script that deploys the new
SuperGoodDollarimplementation and executesupdateCode+setBlockedcalls via Guardian/Safe.
File summaries
| File | Description |
|---|---|
| test/token/SuperGoodDollar.test.ts | Adds blocklist behavior tests and updates fee failure assertions to custom errors. |
| test/token/SuperGoodDollar.nohost.test.ts | Updates fee failure assertions to custom errors in the no-host variant. |
| scripts/upgrades/supergooddollar-block-pools.ts | New upgrade/proposal script to deploy the new implementation and block configured pool addresses. |
| contracts/token/superfluid/SuperGoodDollar.sol | Implements the blocklist (storage + setter + enforcement) and introduces several custom errors. |
| contracts/token/superfluid/ISuperGoodDollar.sol | Exposes isBlocked() and setBlocked() in the public interface. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| * through the agreement layer: a stream can still credit a blocked address, but that | ||
| * address will not be able to move the funds out. | ||
| * | ||
| * usage: yarn hardhat run scripts/upgrades/supergooddollar-block-pools.ts --network <celo|localhost> |
| defaultsDeep({}, ProtocolSettings[networkName], ProtocolSettings["default"]); | ||
|
|
||
| const pools = getPools(); | ||
| if (pools.length === 0) { | ||
| throw new Error( | ||
| `no pools to block for ${networkName}, fill BLOCKED_POOLS in the script or pass BLOCKED_POOLS=0x..,0x.. env` | ||
| ); | ||
| } | ||
|
|
||
| const supergd = await ethers.getContractAt("SuperGoodDollar", release.GoodDollar); | ||
| const owner = await supergd.owner(); | ||
| const host = await supergd.getHost(); | ||
|
|
||
| console.log({ | ||
| networkName, | ||
| root: root.address, | ||
| supergd: supergd.address, | ||
| owner, | ||
| host, | ||
| pools | ||
| }); |
…ance related tests
Description
Adds an owner-managed transfer blocklist to
SuperGoodDollar, so known pools (or anyother address) can be prevented from sending or receiving G$, plus the upgrade script
to deploy it and block an initial set of pools in one proposal.
mapping(address => bool) public isBlocked— new appended storage slot, upgrade-safesetBlocked(address account, bool blocked)— owner only, emitsBlockedUpdatedSUPER_GOODDOLLAR_BLOCKED()The check sits at the top of
_processFees, which is the single choke point every G$movement passes through:
transfer/transferFrom(_transferFrom), ERC777send/operatorSend(_send),transferAndCall, and the Superfluid host batch operations(
operationTransferFrom/operationSendboth route into those). One call site insteadof three, for code-size reasons — see below. The doc comment on
_processFeesflagsthat it is now load-bearing for the blocklist.
Known gaps (both documented on
setBlocked)transfer, so a stream can still credit a blocked address — but that address cannotmove the funds out afterwards. Blocking flows properly would require decoding agreement
data in
createAgreement, which is fragile.pause()remains the tool for a livestream emergency.
mintto a blocked address is still allowed. Minting is already minter-gated, andthe extra check did not fit in the bytecode budget.
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
Add an owner-managed transfer blocklist to SuperGoodDollar and provide the deployment workflow for blocking known pools.
New Features:
Enhancements:
Deployment:
Tests: