Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/contracts/SavingCircles.sol
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuardUpgradeable, OwnableUpg
}

/// @inheritdoc ISavingCircles
function decommission(uint256 _id) external override nonReentrant onlyActive(_id) onlyMember(_id, msg.sender) {
function decommission(uint256 _id) external virtual override nonReentrant onlyActive(_id) onlyMember(_id, msg.sender) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

10. Making decommission virtual is not enough. YieldSavingCircles never overrides it.

The inherited body safeTransfers circle.token amounts from roundDeposits out of this contract. After _deposit those tokens are in CrowdStake (and after the BREAD adapter they have been burned). Members get nothing / the call reverts.

Override must: settle accounting, redeem the correct principal from the module, realize owed yield, convert back to BREAD, then pay members. Also broken further if _withdraw zeros roundDeposits (point 6).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Amendment (author DMs): decommission is not "return each member their yield." At-fault members' unclaimed yield is meant to be spread to not-at-fault members as a reward for staying current.

That is a scope change vs #189 / this PR's "no forfeiture" language, and it cannot be implemented on the inherited body or on global claimableYield[address]. It also depends on roundDeposits remaining intact so "at fault" is computable.

Open: exact at-fault definition, yield-only vs principal, no clawback of Anytime claims, and what happens to keep-staked pots. Full writeup in the conversation amendment.

if (!_isDecommissionable(_id)) revert NotDecommissionable();

address token = circles[_id].token;
Expand Down Expand Up @@ -379,7 +379,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuardUpgradeable, OwnableUpg
* @dev Make a withdrawal from a specified circle.
* Anyone can trigger the payout for a valid member whose turn is withdrawable.
*/
function _withdraw(uint256 _id, address _member) internal onlyMember(_id, _member) {
function _withdraw(uint256 _id, address _member) internal virtual onlyMember(_id, _member) {
Circle storage _circle = circles[_id];

if (!_claimable(_id, _member)) revert NotWithdrawable();
Expand All @@ -406,7 +406,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuardUpgradeable, OwnableUpg
uint256 _id,
uint256 _value,
address _member
) internal onlyCommissioned(_id) onlyMember(_id, _member) {
) internal virtual onlyCommissioned(_id) onlyMember(_id, _member) {
Circle memory _circle = circles[_id];

if (block.timestamp < circles[_id].effectiveCircleStartTime) {
Expand Down
277 changes: 277 additions & 0 deletions src/contracts/YieldSavingCircles.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,277 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;

import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';

import {SavingCircles} from 'contracts/SavingCircles.sol';
import {IYieldModule} from 'interfaces/IYieldModule.sol';

using SafeERC20 for IERC20;

/**
* @title YieldSavingCircles
* @notice A yield-bearing ROSCA. Deposits are routed into a crowdstake.fun yield
* module (sDAI on Gnosis) so principal earns yield while the circle runs.
* @author Breadchain Collective
* @dev ============================ DRAFT / NOT YET COMPILED ============================
* Written without a local Solidity toolchain (agent sandbox was out of disk).
* Compilation, tests, and an accounting/security review are REQUIRED before use.
* Tracking issue: BreadchainCoop/saving-circles#189.
* ================================================================================
*
* Agreed yield model (issue #189):
* - Base = per-member, time-weighted accrual. Each deposit earns yield for the
* depositor for as long as it sits staked -> earlier deposits earn more. This is
* implemented with a MasterChef-style global accumulator over staked principal.
* - Claimant option: when a member claims their round's pot they may either take it
* (redeem principal to their wallet, yield on it stops) or LEAVE IT STAKED so the
* pot keeps compounding for THEM until they pull it (`claim(id, keepStaked=true)`).
* - Claim-timing is modular per circle: yield claimable ANYTIME or only ONCE THE
* CIRCLE ENDS ({ClaimTiming}). Set via {setClaimTiming} before the circle starts.
* - No forfeiture (out of scope for v1).
*
* Token model: v1 treats the circle token as the yield module's underlying
* (identity wrap). To keep BREAD user-facing (burn BREAD -> xDAI -> stake to sDAI,
* and reverse on payout) override {_toUnderlying}/{_fromUnderlying}; the accounting
* below is denominated in the module's underlying units. See #189 gap "BREAD wrap".
*/
contract YieldSavingCircles is SavingCircles {
/// @notice When a member may claim their accrued yield.
enum ClaimTiming {
Anytime,
EndOnly
}

uint256 private constant _ACC_PRECISION = 1e18;

/// @notice The crowdstake.fun yield module (stake pool) principal is staked into.
IYieldModule public yieldModule;

/// @notice Per-circle yield claim-timing policy. Defaults to Anytime (0).
mapping(uint256 id => ClaimTiming timing) public claimTiming;

// --- MasterChef-style global yield accounting (single shared pool) ---
uint256 private _accYieldPerPrincipal; // scaled by _ACC_PRECISION
uint256 private _totalPrincipal; // total principal currently staked across all circles
uint256 private _lastYieldSeen; // module yieldAccrued() already booked into the accumulator
mapping(address member => uint256 principal) private _principalOf;
mapping(address member => uint256 debt) private _rewardDebt;
mapping(address member => uint256 amount) public claimableYield; // credited, awaiting claim
Comment on lines +54 to +60

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

9 + 12. Member state cannot be global if claim policy is per circle; and this mapping is not a UI-safe read.

A shared _accYieldPerPrincipal can be fine if every circle shares one yield source. _principalOf / _rewardDebt / claimableYield cannot: a member in an Anytime circle and an EndOnly circle gets one collapsed claimableYield[alice]. _yieldUnlocked then blocks all of their yield if any active circle is EndOnly.

Need at least:

principal[circleId][member]
rewardDebt[circleId][member]
claimableYield[circleId][member]
keptPotPrincipal[circleId][member]

claimableYield is also settled storage. Fresh sDAI yield only appears after _harvestGlobal + _settle. The UI reading this mapping and treating 0 as "nothing to claim" will disable claim while unsettled yield exists. Expose a view pendingYield(circleId, member) that simulates the accumulator.


/// @dev Per-call flag: whether the claimant keeps their pot staked. Reset after use.
bool private _keepStakedFlag;

event YieldModuleSet(address indexed yieldModule);
event ClaimTimingSet(uint256 indexed id, ClaimTiming timing);
event PotKeptStaked(uint256 indexed id, address indexed member, uint256 amount);
event YieldClaimed(address indexed member, address indexed receiver, uint256 amount);

error YieldModuleNotSet();
error YieldLocked();
error NothingToClaim();

/// @notice Set the yield module. Owner only. Must be set before any deposits.
function setYieldModule(address _yieldModule) external onlyOwner {
if (_yieldModule == address(0)) revert YieldModuleNotSet();
yieldModule = IYieldModule(_yieldModule);
emit YieldModuleSet(_yieldModule);
}
Comment on lines +74 to +79

@ss251 ss251 Aug 19, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

11. "Must be set before any deposits" is only a comment. Owner can setYieldModule while _totalPrincipal, _principalOf, _rewardDebt, and _lastYieldSeen still describe the old position.

Configure yieldModule once during initialization / make it immutable for the lifetime of the deployment. If module migration is required, implement it as an explicit migration/unwind operation with its own invariants.

_totalPrincipal == 0 is not sufficient to prove rotation is safe — outstanding claimableYield, stale _lastYieldSeen, and other economic state can still be tied to the old module.

Same class of bug on setSoleToken (L274): no relationship to circle.token / allowedTokens, and it can change while circles exist.


/// @notice Set a circle's yield claim-timing. Owner of the circle, before it starts.
function setClaimTiming(uint256 _id, ClaimTiming _timing) external {
if (circles[_id].owner != msg.sender) revert NotOwner();
if (isActive[_id]) revert AlreadyActive();
claimTiming[_id] = _timing;
emit ClaimTimingSet(_id, _timing);
}

/**
* @notice Claim the round pot, choosing whether to keep it staked and compounding.
* @param _id The circle id.
* @param _keepStaked If true, the pot stays in the yield module earning for the caller
* (withdraw later via {withdrawStakedPrincipal}); if false it is redeemed to the wallet.
*/
function claim(uint256 _id, bool _keepStaked) external nonReentrant onlyMember(_id, msg.sender) onlyActive(_id) {
_keepStakedFlag = _keepStaked;
_withdraw(_id, msg.sender);
_keepStakedFlag = false;
}
Comment on lines +95 to +99

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

8. Claimant does not control keep-staked. _keepStakedFlag is only set here. SavingCircles.withdrawFor(id, member) (and AutomaticSavingCircles) still call _withdraw with the flag false, so anyone — including automation — can force the pot out of the yield position.

The keep-staked choice has to live in per-circle/per-member storage (or withdrawFor must be overridden / disabled), not a transient flag on this one entrypoint.


/// @notice Claim accrued yield to `_receiver`, subject to the member's circles' claim-timing.
function claimYield(address _receiver) external nonReentrant {
if (address(yieldModule) == address(0)) revert YieldModuleNotSet();
_harvestGlobal();
_settle(msg.sender);
if (!_yieldUnlocked(msg.sender)) revert YieldLocked();

uint256 _amount = claimableYield[msg.sender];
if (_amount == 0) revert NothingToClaim();
claimableYield[msg.sender] = 0;

// Pull yield (underlying) from the module, then hand it to the member.
yieldModule.claimYield(_amount, address(this));
_lastYieldSeen -= _amount; // module's yieldAccrued() dropped by _amount
uint256 _out = _fromUnderlying(_amount);
_payoutToken().safeTransfer(_receiver, _out);
Comment on lines +112 to +116

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

3 + 4. Unauthorized claimer, and the wrong asset comes back even if it were authorized.

AbstractToken.claimYield / pool claimYield revert unless msg.sender == yieldClaimer. Standard CrowdStake wiring sets DistributionManager as that claimer. This PR never makes YieldSavingCircles the claimer.

If it did succeed:

  • token-mode SexyDaiYield.claimYield mints the CrowdStake token to the receiver
  • pool-mode pays WXDAI

Neither is "underlying ERC-20 that _fromUnderlying + _payoutToken().safeTransfer can handle" in the general case. This cannot assume the donated-yield claim path.


emit YieldClaimed(msg.sender, _receiver, _out);
}

/// @notice Withdraw principal a claimant previously left staked (keepStaked path).
function withdrawStakedPrincipal(address _receiver) external nonReentrant {
if (address(yieldModule) == address(0)) revert YieldModuleNotSet();
_harvestGlobal();
_settle(msg.sender);

uint256 _principal = _principalOf[msg.sender];
if (_principal == 0) revert NothingToClaim();
_principalOf[msg.sender] = 0;
_totalPrincipal -= _principal;
_rewardDebt[msg.sender] = 0;

yieldModule.burn(_principal, address(this));
uint256 _out = _fromUnderlying(_principal);
_payoutToken().safeTransfer(_receiver, _out);
Comment on lines +121 to +135

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

7. This withdraws all of the caller's principal, not the keep-staked pot.

_deposit also increments _principalOf[member]. There is no circleId and no keptPotPrincipal. A member can pull deposits that are still backing active rounds; roundDeposits still says those contributions exist, then the later pot claim subtracts that principal again.

Needs per-circle balances: mandatory deposits vs voluntarily retained pot. withdrawStakedPrincipal(id, receiver) should burn only keptPotPrincipal[id][msg.sender].

}

// =========================================================================
// Overrides
// =========================================================================

/// @dev Route the just-deposited principal into the yield module and book it for the member.
function _deposit(uint256 _id, uint256 _value, address _member) internal override {
if (address(yieldModule) == address(0)) revert YieldModuleNotSet();

_harvestGlobal();
_settle(_member);

// Base pulls `_value` of the circle token into this contract and updates ROSCA bookkeeping.
super._deposit(_id, _value, _member);

uint256 _underlyingAmount = _toUnderlying(_value);
_underlyingToken().forceApprove(address(yieldModule), _underlyingAmount);
yieldModule.mint(_underlyingAmount, address(this));

@ss251 ss251 Aug 19, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1. Wrong mint ABI. This encodes mint(uint256,address). The current concrete CrowdStake token/pool implementations expose mint(address,uint256) only, so deposits against the current concrete CrowdStake token/pool implementations revert.

Do not keep this as IYieldModule.mint. Principal deposit/withdraw should go through IStakePool / IToken (mint(address,uint256) / burn(uint256,address)). Leave IYieldModule for yield inspection / claiming.


_principalOf[_member] += _underlyingAmount;
_totalPrincipal += _underlyingAmount;
_rewardDebt[_member] = (_principalOf[_member] * _accYieldPerPrincipal) / _ACC_PRECISION;
}

/**
* @dev Claimant takes their round's pot. Each depositor's yield on that round is settled to
* them (kept per Model B); the pot principal either redeems to the claimant, or (Model A)
* is re-attributed to the claimant and kept staked to keep compounding.
*/
function _withdraw(uint256 _id, address _member) internal override onlyMember(_id, _member) {
if (!_claimable(_id, _member)) revert NotWithdrawable();

_harvestGlobal();

uint256 _round = _memberStates[_id][_member].memberIndex; // claimant's turn == their index
address[] memory _members = circleMembers[_id];
uint256 _pot;

for (uint256 i = 0; i < _members.length; i++) {
address _depositor = _members[i];
uint256 _d = roundDeposits[_id][_round][_depositor];
if (_d == 0) continue;

uint256 _underlyingD = _toUnderlying(_d);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

5. _toUnderlying is not a safe BREAD-adapter hook. It is invoked here on historical roundDeposits during withdraw, and again on a live deposit at L152.

BREAD conversion is stateful (BREAD burn -> native xDAI, reverse on payout). If an override burns BREAD, this path burns the same historical amount a second time.

Split:

  • asset movement/conversion (deposit/payout only)
  • amount/accounting conversion (pure, idempotent, safe to re-run on history)


// Credit this depositor the yield their principal earned, then remove this round's principal.
_settle(_depositor);
_principalOf[_depositor] -= _underlyingD;
_totalPrincipal -= _underlyingD;
_rewardDebt[_depositor] = (_principalOf[_depositor] * _accYieldPerPrincipal) / _ACC_PRECISION;

roundDeposits[_id][_round][_depositor] = 0; // consumed into the pot

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

6. Do not consume roundDeposits. Inherited _deposit / _isDecommissionable treat these values as "was the previous round fully funded?".

Zeroing them after the round-0 claim makes round 1 look like an incomplete previous round → CircleTimedOut() / decommissionable. Keep roundDeposits as immutable ROSCA history and track yield-principal redemption in separate storage.

Suggested change
roundDeposits[_id][_round][_depositor] = 0; // consumed into the pot

_pot += _underlyingD;
}

_memberStates[_id][_member].hasClaimed = true;

if (_keepStakedFlag) {
// Pot never leaves the pool; re-attribute it to the claimant so it keeps earning for them.
_settle(_member);
_principalOf[_member] += _pot;
_totalPrincipal += _pot;
_rewardDebt[_member] = (_principalOf[_member] * _accYieldPerPrincipal) / _ACC_PRECISION;
emit PotKeptStaked(_id, _member, _pot);
} else {
yieldModule.burn(_pot, address(this));
uint256 _out = _fromUnderlying(_pot);
_payoutToken().safeTransfer(_member, _out);
Comment on lines +201 to +204

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

4. burn remits native xDAI, not an ERC-20. Both SexyDaiYield._remit and PoolNativeYield._remit do sDAI -> WXDAI -> native xDAI and _nativeTransfer(receiver_, amount_).

This contract has no receive() / fallback(), so the native transfer reverts. Even if it accepted ETH, _fromUnderlying + _payoutToken().safeTransfer would still treat a native credit as if an ERC-20 arrived.

Same bug on withdrawStakedPrincipal (burn at L133). Principal vs yield vs token-mode claim are three different asset flows; a single _fromUnderlying(uint256) seam cannot represent them.

}

emit FundsWithdrawn(_id, _member, _pot);

if (_allMembersClaimed(_id)) {
isActive[_id] = false;
}
}

// =========================================================================
// Internals
// =========================================================================

/// @dev Book newly accrued module yield into the global per-principal accumulator.
function _harvestGlobal() internal {
if (_totalPrincipal == 0) return;
uint256 _accruedNow = yieldModule.yieldAccrued();
if (_accruedNow > _lastYieldSeen) {
uint256 _delta = _accruedNow - _lastYieldSeen;
_accYieldPerPrincipal += (_delta * _ACC_PRECISION) / _totalPrincipal;
_lastYieldSeen = _accruedNow;
}
Comment on lines +218 to +226

@ss251 ss251 Aug 19, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

2. Wrong yield source. yieldAccrued() is CrowdStake's donated/distributable pool, not this contract's holder yield.

CrowdStake splits:

- yieldAccrued() — donated/distributable yield
- claimYield(...) — consumes donated yield; restricted to yieldClaimer
- totalYieldAccrued() — full vault surplus
- keptYieldOf(account) / claimKeptYield() — holder-owned

yieldAccrued() itself is a public view. The claimer restriction is on claimYield(), which reverts unless msg.sender == yieldClaimer.

If this module is shared, other holders' donated yield enters _delta while their principal is not in _totalPrincipal. SavingCircles needs an explicit ownership path (dedicated instance + claimer, or 100% kept-yield as the holder) before this accumulator is meaningful.

}

/// @dev Move a member's pending accumulator yield into their claimable balance.
function _settle(address _member) internal {
uint256 _accumulated = (_principalOf[_member] * _accYieldPerPrincipal) / _ACC_PRECISION;
uint256 _pending = _accumulated - _rewardDebt[_member];
if (_pending > 0) claimableYield[_member] += _pending;
_rewardDebt[_member] = _accumulated;
}

/// @dev A member's yield is locked while any of their circles is EndOnly and still active.
function _yieldUnlocked(address _member) internal view returns (bool) {
uint256[] memory _ids = memberCircles[_member];
for (uint256 i = 0; i < _ids.length; i++) {
if (isActive[_ids[i]] && claimTiming[_ids[i]] == ClaimTiming.EndOnly) return false;
}
return true;
}
Comment on lines +237 to +244

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

9. ClaimTiming is broken by global member state. Any active EndOnly circle locks the member's entire claimableYield, including yield earned in an Anytime circle. This only becomes correct once yield is circle-scoped.


// =========================================================================
// Wrap seams — override to keep BREAD user-facing (BREAD<->xDAI<->sDAI)
// =========================================================================

/// @dev Convert a circle-token amount into the yield module's underlying. Identity in v1.
function _toUnderlying(uint256 _amount) internal virtual returns (uint256) {

Check warning on line 251 in src/contracts/YieldSavingCircles.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Function order is incorrect, internal function can not go after internal view function (line 238)
return _amount;
}

/// @dev Convert an underlying amount back into the circle/payout token. Identity in v1.
function _fromUnderlying(uint256 _amount) internal virtual returns (uint256) {
return _amount;
}

/// @dev The ERC20 the yield module pulls on mint / returns on burn. v1: the payout token.
function _underlyingToken() internal view virtual returns (IERC20) {
return _payoutToken();
}

/// @dev The ERC20 members are paid in (principal + yield). v1: single supported token.
function _payoutToken() internal view virtual returns (IERC20) {
return IERC20(_soleToken);
}

/// @dev v1 single-token constraint: set on first allowed token. See #189 (multi-token later).
address private _soleToken;

/// @notice Owner sets the single supported ERC20 for circles + payouts (v1 constraint).
function setSoleToken(address _token) external onlyOwner {
_soleToken = _token;
}
Comment on lines +273 to +276

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

11. setSoleToken has no invariant. Inherited create() still accepts any allowedTokens entry. Nothing requires circle.token == _soleToken. Owner can retarget payouts while circles are live. Make this immutable at initialize, or require no active circles / no principal, and reject create when token != _soleToken.

}
30 changes: 30 additions & 0 deletions src/interfaces/IYieldModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;

/**
* @title IYieldModule
* @notice Minimal surface of a crowdstake.fun yield module / stake pool that the
* saving-circles integration consumes.
* @dev Mirrors `BreadchainCoop/crowdstake.fun` `contracts/src/interfaces/IYieldModule.sol`.
* The concrete Gnosis implementation is `PoolNativeYield` / `SexyDaiYield`
* (WXDAI/xDAI -> sDAI ERC-4626), deployed per `contracts/deployments/gnosis.json`.
*
* Deposit model: `mint(amount, receiver)` pulls `amount` of the module's
* UNDERLYING asset from the caller and credits `receiver` with principal.
* `burn(amount, receiver)` redeems `amount` of principal and sends the
* underlying to `receiver`. Yield accrues on top of principal and is read
* via `yieldAccrued()` and pulled via `claimYield(amount, receiver)`.
*/
interface IYieldModule {
/// @notice Stake `amount` of the underlying (pulled from caller) crediting `receiver`.
function mint(uint256 amount, address receiver) external;
Comment on lines +12 to +20

@ss251 ss251 Aug 19, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1. Interface drift. This mint(uint256,address) selector (0x94bf804d) is not what the concrete CrowdStake token/pool implements.

The current concrete CrowdStake token/pool implementations expose mint(address receiver, uint256 amount) (0x40c10f19):

The selector mismatch proves this local IYieldModule is not the canonical deposit interface. Rather than changing IYieldModule.mint(...), use CrowdStake’s actual deposit surface (IStakePool / IToken) for principal operations, and keep IYieldModule limited to yield-related reads/claims if that’s its intended role.

deposit / withdraw principal
→ IStakePool or IToken

yield inspection / claiming
→ IYieldModule


/// @notice Redeem `amount` of principal, sending the underlying asset to `receiver`.
function burn(uint256 amount, address receiver) external;

/// @notice Claim `amount` of accrued yield, sending the underlying asset to `receiver`.
function claimYield(uint256 amount, address receiver) external;

/// @notice Total yield currently accrued to this holder and available to claim.
function yieldAccrued() external view returns (uint256);
}
Loading