feat: implement pausable streams in SuperGoodDollar contract - #303
feat: implement pausable streams in SuperGoodDollar contract#303blueogin wants to merge 12 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 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="contracts/token/superfluid/SuperGoodDollar.sol" line_range="132" />
<code_context>
+ // the CFA packs int96 flowRate at bits [128,224) of the first word
+ bool increased;
+ assembly {
+ let newRate := signextend(11, shr(128, calldataload(data.offset)))
+ let oldRate := signextend(11, shr(128, sload(slot)))
+ increased := sgt(newRate, oldRate)
+ }
+ if (increased) revert SUPER_GOODDOLLAR_PAUSED();
</code_context>
<issue_to_address>
**issue (bug_risk):** `oldRate` is loaded from `sload(slot)`, but `FixedSizeData.storeData` stores the array length at `slot` and stores the first agreement-data word at `slot + 1`. For an existing CFA flow, `oldRate` is therefore the data length (normally `2`), so decreasing a normal positive flow to another positive rate is treated as an increase and reverts while paused.
**Triggers:** When an existing flow is decreased, rather than closed, while the token is paused.
**Suggested fix:** Load the first stored agreement-data word with `sload(add(slot, 1))` before extracting the packed flow rate.
```suggestion
let oldRate := signextend(11, shr(128, sload(add(slot, 1))))
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and while paused, this changes whether Superfluid can create or increase token streams, and a faulty calldata/storage decoding or agreement-slot assumption could either leave transfers running during an incident or block needed flows. Reverting restores the gate, but any tokens transferred by an incorrectly permitted stream may not be recoverable.
Blocking findings: contracts/token/superfluid/SuperGoodDollar.sol:132
There was a problem hiding this comment.
🟡 Changes recommended
The new pause guard decodes newRate incorrectly because it reads the calldata array length instead of data[0], so it won’t reliably block opening/increasing streams.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes SuperGoodDollar’s pause enforcement for Superfluid Constant Flow Agreements (CFA) by moving the pause guard onto the updateAgreementData path that CFA actually uses, while preserving the ability to decrease/close/liquidate existing streams during an incident.
Changes:
- Add a
SuperfluidTokenhook (_beforeAgreementDataUpdate) invoked fromupdateAgreementDatabefore writing agreement data. - Implement pause-aware CFA flow-rate gating in
SuperGoodDollarby comparing the newly written flow rate vs the stored one and rejecting only increases while paused. - Add tests to ensure new streams and stream increases revert while paused, but closing streams remains possible.
File summaries
| File | Description |
|---|---|
| test/token/SuperGoodDollar.test.ts | Adds regression tests for pausing behavior across create/update/delete flow operations. |
| contracts/token/superfluid/SuperGoodDollar.sol | Implements the pause guard on agreement-data updates by decoding and comparing flow rates. |
| contracts/token/superfluid/SuperfluidToken.sol | Introduces a reusable pre-write hook for updateAgreementData to enable token-specific gating. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
Can the DAO also have permissions to delete a flow?
| if (paused() && msg.sender == _cfaV1()) { | ||
| _onlyNotIncreasingFlow(slot, data); | ||
| } | ||
| FixedSizeData.storeData(slot, data); |
There was a problem hiding this comment.
why not use super.updateAgreementData?
There was a problem hiding this comment.
@hellwolf
SuperfluidToken.updateAgreemenData is external, so super. can't reach them.
If you are ok, I can make it public but there is another reason:
Using public + super costs more contract size which I do not want
…reverify schedule test
|
@sirpy |
…ate errors" This reverts commit 3926686.
| * 2. added allowHostOperations to disable host actions by G$ governance in case of security issues | ||
| * 3. made updateAgreementData virtual (so SuperGoodDollar can gate streams while paused) | ||
| */ | ||
| abstract contract SuperfluidToken is ISuperfluidToken { |
There was a problem hiding this comment.
I am just curious here: Does the codebase keep tracking the upstream code? Since I am wary of divergence of code.
Description
pause()did not stop streaming. The guard sat oncreateAgreement, which the CFA never calls when a flow is opened — it writes all flow state throughupdateAgreementData. Anyone could open, increase, or keep running a G$ stream while the token was paused.This moves the guard to the path the CFA actually uses, and gates it so that opening or increasing a flow is blocked while paused, but closing, decreasing and liquidating stay available.
The bug
Verified against the installed
@superfluid-finance/ethereum-contracts@1.8.1.ConstantFlowAgreementV1calls, on the token:updateAgreementData— for create, update and delete (line 1402, plus the ACL paths at 862/1079)updateAgreementStateSlot— account flow state (line 1104)settleBalance— line 1097It never calls
createAgreement, and never callsterminateAgreement. A flow is deleted by writing the same agreement data back withflowRate = 0.So the existing
_onlyNotPaused()oncreateAgreementprotected nothing on the streaming path. (It is not dead code — the IDA callscreateAgreementfor index and subscription creation — so the guard is kept, with a corrected comment.)The fix
Because create and delete share one code path, a blanket pause on
updateAgreementDatawould also block shutting malicious streams down — the opposite of what is needed during an incident. The guard therefore compares the new flow rate against the stored one and rejects only an increase:+n > 0+2n > +n+n/2 < +n0 < +ndeleteFlow)0 < +nsignextend(11, ...)sign-extends from bit 95, discarding the timestamp packed at bits 224+ and recovering theint96flow rate. An unset slot reads0, so a fresh flow is+n > 0and is blocked.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
Enforce pausing on new and increased SuperGoodDollar streams while keeping stream shutdown operations available during incidents.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: