Skip to content

feat: block transfers to/from known pools in SuperGoodDollar - #306

Merged
blueogin merged 6 commits into
masterfrom
feat/supergooddollar-block-pools
Sep 24, 2026
Merged

blueogin merged 6 commits into
masterfrom
feat/supergooddollar-block-pools

Conversation

@blueogin

@blueogin blueogin commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Adds an owner-managed transfer blocklist to SuperGoodDollar, so known pools (or any
other 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-safe
  • setBlocked(address account, bool blocked) — owner only, emits BlockedUpdated
  • Blocked transfers revert with SUPER_GOODDOLLAR_BLOCKED()

The check sits at the top of _processFees, which is the single choke point every G$
movement passes through: transfer / transferFrom (_transferFrom), ERC777 send /
operatorSend (_send), transferAndCall, and the Superfluid host batch operations
(operationTransferFrom / operationSend both route into those). One call site instead
of three, for code-size reasons — see below. The doc comment on _processFees flags
that it is now load-bearing for the blocklist.

Known gaps (both documented on setBlocked)

  • Streams are not blocked. CFA settles through the agreement layer rather than
    transfer, so a stream can still credit a blocked address — but that address cannot
    move the funds out afterwards. Blocking flows properly would require decoding agreement
    data in createAgreement, which is fragile. pause() remains the tool for a live
    stream emergency.
  • mint to a blocked address is still allowed. Minting is already minter-gated, and
    the 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:

  • PR title matches follow: (Feature|Bug|Chore) Task Name
  • My code follows the style guidelines of this project
  • I have followed all the instructions described in the initial task (check Definitions of Done)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added reference to a related issue in the repository
  • I have added a detailed description of the changes proposed in the pull request. I am as descriptive as possible, assisting reviewers as much as possible.
  • I have added screenshots related to my pull request (for frontend tasks)
  • I have pasted a gif showing the feature.
  • @mentions of the person or team responsible for reviewing proposed changes

Summary by Sourcery

Add an upgrade-safe SuperGoodDollar address blocklist and deploy it with governance tooling for managing known liquidity pools.

New Features:

  • Add owner-managed blocking and unblocking of addresses across SuperGoodDollar transfer surfaces, with events and blocked-transfer errors.
  • Provide an upgrade script that deploys the new implementation and proposes atomically blocking configured pools through the DAO governance flow.

Enhancements:

  • Replace several string-based validation failures with custom errors and enforce blocklist checks for minting, burning, and transfers.
  • Support configurable JSON or CSV pool lists with validation, simulation, and post-execution verification.

Deployment:

  • Add deployment configuration for the initial blocked-pool set and an upgrade workflow for production and fork simulations.

Tests:

  • Test owner access, batch updates, transfer blocking and unblocking, ERC20/ERC677/ERC777 paths, minting, burning, and administrative burns.

Chores:

  • Update existing fee-related tests to assert custom errors and simplify identity test time handling.

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>
Copilot AI lite review requested due to automatic review settings September 10, 2026 15:20

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread scripts/upgrades/supergooddollar-block-pools.ts Outdated
Comment thread contracts/token/superfluid/SuperGoodDollar.sol

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 isBlocked mapping + setBlocked() (owner-only) and enforce blocking at the _processFees choke point.
  • Replace several revert strings with custom errors (including fee-balance failures) and update tests accordingly.
  • Add an upgrade script that deploys the new SuperGoodDollar implementation and executes updateCode + setBlocked calls 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.

Comment thread scripts/upgrades/supergooddollar-block-pools.ts Outdated
Comment thread scripts/upgrades/supergooddollar-block-pools.ts Outdated
@blueogin
blueogin requested a review from sirpy September 16, 2026 15:19
sirpy
sirpy previously requested changes Sep 16, 2026
Comment thread contracts/token/superfluid/SuperGoodDollar.sol
@blueogin
blueogin requested a review from sirpy September 16, 2026 18:06
@blueogin
blueogin dismissed sirpy’s stale review September 22, 2026 14:19

The changes are already deployed

@blueogin
blueogin merged commit f5fce71 into master Sep 24, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants