From e5f942f541e3c277169ba5faaa4d3dab514a364b Mon Sep 17 00:00:00 2001 From: bagelface Date: Thu, 26 Dec 2024 12:41:13 -0500 Subject: [PATCH] feat: allow non-owner members to decommission if deposits are not received on time --- src/contracts/SavingCircles.sol | 82 +++++++++++++++++++--------- src/interfaces/ISavingCircles.sol | 16 +++--- test/integration/SavingCircles.t.sol | 49 +++++++++++++---- test/unit/SavingCirclesUnit.t.sol | 67 ++++++++++++++++------- test/unit/SavingCirclesUnit.tree | 2 +- 5 files changed, 149 insertions(+), 67 deletions(-) diff --git a/src/contracts/SavingCircles.sol b/src/contracts/SavingCircles.sol index 398fbc6..1f0c62a 100644 --- a/src/contracts/SavingCircles.sol +++ b/src/contracts/SavingCircles.sol @@ -9,7 +9,7 @@ import {ISavingCircles} from '../interfaces/ISavingCircles.sol'; /** * @title Saving Circles - * @notice TODO + * @notice Simple implementation of a rotating savings and credit association (ROSCA) for ERC20 tokens * @author Breadchain Collective * @author @RonTuretzky * @author @bagelface @@ -32,10 +32,10 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { } /** - * @notice Commission a new saving circle + * @notice Create a new saving circle * @param _circle A new saving circle */ - function addCircle(Circle memory _circle) external override { + function create(Circle memory _circle) external override { bytes32 _id = keccak256(abi.encodePacked(_circle.name)); if (circles[_id].owner != address(0)) revert AlreadyExists(); @@ -63,17 +63,17 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { * @param _value Amount of the token to deposit */ function deposit(bytes32 _id, uint256 _value) external override nonReentrant { - _deposit(_id, msg.sender, _value); + _deposit(_id, _value, msg.sender); } /** * @notice Make a deposit on behalf of another member * @param _id Identifier of the circle - * @param _member Address to make a deposit for * @param _value Amount of the token to deposit + * @param _member Address to make a deposit for */ - function depositFor(bytes32 _id, address _member, uint256 _value) external override nonReentrant { - _deposit(_id, _member, _value); + function depositFor(bytes32 _id, uint256 _value, address _member) external override nonReentrant { + _deposit(_id, _value, _member); } /** @@ -85,6 +85,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { if (!_withdrawable(_id)) revert NotWithdrawable(); if (_circle.members[_circle.currentIndex] != msg.sender) revert NotWithdrawable(); + if (_circle.currentIndex >= _circle.maxDeposits) revert NotWithdrawable(); uint256 _withdrawAmount = _circle.depositAmount * (_circle.members.length); @@ -96,7 +97,7 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { bool success = IERC20(_circle.token).transfer(msg.sender, _withdrawAmount); if (!success) revert TransferFailed(); - emit WithdrawalMade(_id, msg.sender, _withdrawAmount); + emit FundsWithdrawn(_id, msg.sender, _withdrawAmount); } /** @@ -115,17 +116,33 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { * @dev Returns all deposits to members * @param _id Identifier of the circle */ - function decommissionCircle(bytes32 _id) external override { + function decommission(bytes32 _id) external override { Circle storage _circle = circles[_id]; - if (_circle.owner != msg.sender) revert NotOwner(); + if (_circle.owner != msg.sender) { + if (!isMember[_id][msg.sender]) revert NotMember(); + if (block.timestamp <= _circle.circleStart + (_circle.depositInterval * (_circle.currentIndex + 1))) { + revert NotDecommissionable(); + } + + bool hasIncompleteDeposits = false; + for (uint256 i = 0; i < _circle.members.length; i++) { + if (balances[_id][_circle.members[i]] < _circle.depositAmount) { + hasIncompleteDeposits = true; + break; + } + } + if (!hasIncompleteDeposits) revert NotDecommissionable(); + } + // Return deposits to members for (uint256 i = 0; i < _circle.members.length; i++) { address _member = _circle.members[i]; uint256 _balance = balances[_id][_member]; if (_balance > 0) { balances[_id][_member] = 0; - IERC20(_circle.token).transfer(_member, _balance); + bool success = IERC20(_circle.token).transfer(_member, _balance); + if (!success) revert TransferFailed(); } } @@ -157,10 +174,12 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { } /** - * @notice TODO - * @param _id TODO + * @notice Return the balances of the members of a specified saving circle + * @param _id Identifier of the circle + * @return _members Members of the specified saving circle + * @return _balances Corresponding balances of the members of the circle */ - function balancesForCircle(bytes32 _id) + function memberBalances(bytes32 _id) external view override @@ -179,28 +198,33 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { } /** - * @notice TODO - * @param _id TODO - * @param _member TODO + * @notice Return the member address which is currently able to withdraw from a specified circle + * @param _id Identifier of the circle + * @return address Member that is currently able to withdraw from the circle */ - function withdrawableBy(bytes32 _id, address _member) external view override returns (bool) { + function withdrawableBy(bytes32 _id) external view override returns (address) { Circle memory _circle = circles[_id]; if (_isDecommissioned(_circle)) revert NotCommissioned(); - if (!isMember[_id][_member]) revert NotMember(); - return _circle.members[_circle.currentIndex] == _member; + return _circle.members[_circle.currentIndex]; } /** - * @notice TODO - * @param _id TODO + * @notice Return if a circle can currently be withdrawn from + * @param _id Identifier of the circle + * @return bool If the circle is able to be withdrawn from */ function withdrawable(bytes32 _id) external view override returns (bool) { return _withdrawable(_id); } - function _deposit(bytes32 _id, address _member, uint256 _value) internal { + /** + * @dev Make a deposit into a specified circle + * A deposit must be made in specific time window and can be made partially so long as the final balance equals + * the specified deposit amount for the circle. + */ + function _deposit(bytes32 _id, uint256 _value, address _member) internal { Circle memory _circle = circles[_id]; if (_isDecommissioned(_circle)) revert NotCommissioned(); @@ -217,20 +241,23 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { bool success = IERC20(_circle.token).transferFrom(msg.sender, address(this), _value); if (!success) revert TransferFailed(); - emit DepositMade(_id, _member, _value); + emit FundsDeposited(_id, _member, _value); } + /** + * @dev Return if a specified circle is withdrawable + * To be considered withdrawable, enough time must have passed since the deposit interval started + * and all members must have made a deposit. + */ function _withdrawable(bytes32 _id) internal view returns (bool) { Circle memory _circle = circles[_id]; if (_isDecommissioned(_circle)) revert NotCommissioned(); - // Check if enough time has passed since circle start for current withdrawal if (block.timestamp < _circle.circleStart + (_circle.depositInterval * _circle.currentIndex)) { return false; } - // Check if all members have made their initial deposit for (uint256 i = 0; i < _circle.members.length; i++) { if (balances[_id][_circle.members[i]] < _circle.depositAmount) { return false; @@ -240,6 +267,9 @@ contract SavingCircles is ISavingCircles, ReentrancyGuard, OwnableUpgradeable { return true; } + /** + * @dev Return if a specified circle is decommissioned by checking if an owner is set + */ function _isDecommissioned(Circle memory _circle) internal pure returns (bool) { return _circle.owner == address(0); } diff --git a/src/interfaces/ISavingCircles.sol b/src/interfaces/ISavingCircles.sol index db94a68..00c6b07 100644 --- a/src/interfaces/ISavingCircles.sol +++ b/src/interfaces/ISavingCircles.sol @@ -18,8 +18,8 @@ interface ISavingCircles { bytes32 indexed id, string name, address[] members, address token, uint256 depositAmount, uint256 depositInterval ); event CircleDecommissioned(bytes32 indexed id); - event DepositMade(bytes32 indexed id, address indexed contributor, uint256 amount); - event WithdrawalMade(bytes32 indexed id, address indexed withdrawer, uint256 amount); + event FundsDeposited(bytes32 indexed id, address indexed member, uint256 amount); + event FundsWithdrawn(bytes32 indexed id, address indexed member, uint256 amount); event TokenAllowed(address indexed token, bool indexed allowed); error AlreadyDeposited(); @@ -28,23 +28,23 @@ interface ISavingCircles { error InvalidCircle(); error NotCommissioned(); error NotMember(); - error NotOwner(); + error NotDecommissionable(); error NotWithdrawable(); error TransferFailed(); // External functions (state-changing) function initialize(address owner) external; function setTokenAllowed(address token, bool allowed) external; - function addCircle(Circle memory circle) external; + function create(Circle memory circle) external; function deposit(bytes32 id, uint256 value) external; - function depositFor(bytes32 id, address member, uint256 value) external; + function depositFor(bytes32 id, uint256 value, address member) external; function withdraw(bytes32 id) external; - function decommissionCircle(bytes32 id) external; + function decommission(bytes32 id) external; // External view functions function circle(bytes32 id) external view returns (Circle memory); function isTokenAllowed(address token) external view returns (bool); - function balancesForCircle(bytes32 id) external view returns (address[] memory, uint256[] memory); + function memberBalances(bytes32 id) external view returns (address[] memory, uint256[] memory); function withdrawable(bytes32 id) external view returns (bool); - function withdrawableBy(bytes32 id, address member) external view returns (bool); + function withdrawableBy(bytes32 id) external view returns (address); } diff --git a/test/integration/SavingCircles.t.sol b/test/integration/SavingCircles.t.sol index 90fcdcd..7f939d7 100644 --- a/test/integration/SavingCircles.t.sol +++ b/test/integration/SavingCircles.t.sol @@ -83,7 +83,7 @@ contract SavingCirclesIntegration is Test { circle.setTokenAllowed(address(token), true); vm.prank(alice); - circle.addCircle(baseCircle); + circle.create(baseCircle); } function test_SetTokenAllowed() public { @@ -128,7 +128,7 @@ contract SavingCirclesIntegration is Test { baseCircle.token = badToken; vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.InvalidCircle.selector)); - circle.addCircle(baseCircle); + circle.create(baseCircle); } function test_Deposit() public { @@ -137,7 +137,7 @@ contract SavingCirclesIntegration is Test { vm.prank(alice); circle.deposit(BASE_CIRCLE_ID, DEPOSIT_AMOUNT); - (, uint256[] memory balances) = circle.balancesForCircle(BASE_CIRCLE_ID); + (, uint256[] memory balances) = circle.memberBalances(BASE_CIRCLE_ID); assertEq(balances[0], DEPOSIT_AMOUNT); } @@ -146,9 +146,9 @@ contract SavingCirclesIntegration is Test { // Bob deposits for Alice vm.prank(bob); - circle.depositFor(BASE_CIRCLE_ID, alice, DEPOSIT_AMOUNT); + circle.depositFor(BASE_CIRCLE_ID, DEPOSIT_AMOUNT, alice); - (, uint256[] memory balances) = circle.balancesForCircle(BASE_CIRCLE_ID); + (, uint256[] memory balances) = circle.memberBalances(BASE_CIRCLE_ID); assertEq(balances[0], DEPOSIT_AMOUNT); } @@ -208,7 +208,7 @@ contract SavingCirclesIntegration is Test { // Decommission circle vm.prank(alice); - circle.decommissionCircle(BASE_CIRCLE_ID); + circle.decommission(BASE_CIRCLE_ID); // Check balances returned assertEq(token.balanceOf(alice) - aliceBalanceBefore, DEPOSIT_AMOUNT); @@ -219,12 +219,39 @@ contract SavingCirclesIntegration is Test { circle.circle(BASE_CIRCLE_ID); } - function test_RevertWhen_NonOwnerDecommissions() public { + function test_MemberDecommissionWhenIncompleteDeposits() public { createBaseCircle(); - vm.prank(bob); - vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotOwner.selector)); - circle.decommissionCircle(BASE_CIRCLE_ID); + // Only Alice deposits + vm.prank(alice); + circle.deposit(BASE_CIRCLE_ID, DEPOSIT_AMOUNT); + + // Get initial balance + uint256 aliceBalanceBefore = token.balanceOf(alice); + + // Wait until after deposit interval + vm.warp(block.timestamp + DEPOSIT_INTERVAL + 1); + + // Alice should be able to decommission since not all members deposited + vm.prank(alice); + vm.expectEmit(true, true, true, true); + emit ISavingCircles.CircleDecommissioned(BASE_CIRCLE_ID); + circle.decommission(BASE_CIRCLE_ID); + + // Check Alice got her deposit back + assertEq(token.balanceOf(alice) - aliceBalanceBefore, DEPOSIT_AMOUNT); + + // Check circle was deleted + vm.expectRevert(ISavingCircles.NotCommissioned.selector); + circle.circle(BASE_CIRCLE_ID); + } + + function test_RevertWhen_NonMemberDecommissions() public { + createBaseCircle(); + + vm.prank(makeAddr('stranger')); + vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotMember.selector)); + circle.decommission(BASE_CIRCLE_ID); } function test_RevertWhen_NotEnoughContributions() public { @@ -256,7 +283,7 @@ contract SavingCirclesIntegration is Test { // members[2] = carol; // vm.prank(alice); - // circle.addCircle("Test Circle", members, address(token), DEPOSIT_AMOUNT, DEPOSIT_INTERVAL); + // circle.create("Test Circle", members, address(token), DEPOSIT_AMOUNT, DEPOSIT_INTERVAL); // bytes32 hashedName = keccak256(abi.encodePacked("Test Circle")); // // Branch 2: Not enough time passed diff --git a/test/unit/SavingCirclesUnit.t.sol b/test/unit/SavingCirclesUnit.t.sol index 4b84654..d201ce8 100644 --- a/test/unit/SavingCirclesUnit.t.sol +++ b/test/unit/SavingCirclesUnit.t.sol @@ -77,7 +77,7 @@ contract SavingCirclesUnit is Test { // Create an initial test circle vm.prank(alice); - savingCircles.addCircle(baseCircle); + savingCircles.create(baseCircle); } function test_SetTokenAllowedWhenCallerIsNotOwner() external { @@ -145,7 +145,7 @@ contract SavingCirclesUnit is Test { // Expect deposit event vm.expectEmit(true, true, true, true); - emit ISavingCircles.DepositMade(baseCircleId, alice, DEPOSIT_AMOUNT); + emit ISavingCircles.FundsDeposited(baseCircleId, alice, DEPOSIT_AMOUNT); savingCircles.deposit(baseCircleId, DEPOSIT_AMOUNT); @@ -248,14 +248,14 @@ contract SavingCirclesUnit is Test { // First member (alice) should be able to withdraw vm.prank(alice); vm.expectEmit(true, true, true, true); - emit ISavingCircles.WithdrawalMade(baseCircleId, alice, withdrawAmount); + emit ISavingCircles.FundsWithdrawn(baseCircleId, alice, withdrawAmount); savingCircles.withdraw(baseCircleId); // Verify alice received the tokens assertEq(token.balanceOf(alice), withdrawAmount); // Verify all member balances were reset - (, uint256[] memory balances) = savingCircles.balancesForCircle(baseCircleId); + (, uint256[] memory balances) = savingCircles.memberBalances(baseCircleId); for (uint256 i = 0; i < balances.length; i++) { assertEq(balances[i], 0); } @@ -282,29 +282,54 @@ contract SavingCirclesUnit is Test { assertEq(_circle.depositInterval, DEPOSIT_INTERVAL); } - function test_DecommissionWhenCallerIsNotOwner() external { - vm.prank(alice); - vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotOwner.selector)); - savingCircles.decommissionCircle(baseCircleId); + function test_DecommissionWhenOwner() external { + vm.prank(owner); + vm.expectEmit(true, true, true, true); + emit ISavingCircles.CircleDecommissioned(baseCircleId); + savingCircles.decommission(baseCircleId); + + vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotCommissioned.selector)); + savingCircles.circle(baseCircleId); } - function test_DecommissionWhenParametersAreValid() external { - vm.prank(owner); + function test_DecommissionWhenNotMember() external { + vm.prank(makeAddr('stranger')); + vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotMember.selector)); + savingCircles.decommission(baseCircleId); + } + + function test_DecommissionWhenMemberAndIncompleteDeposits() external { + // Have alice deposit but not bob or carol + token.mint(alice, DEPOSIT_AMOUNT); + vm.startPrank(alice); + token.approve(address(savingCircles), DEPOSIT_AMOUNT); + savingCircles.deposit(baseCircleId, DEPOSIT_AMOUNT); + vm.stopPrank(); + + // Warp past deposit interval + vm.warp(block.timestamp + DEPOSIT_INTERVAL + 1); + + // Member should be able to decommission since not all deposits were made + vm.prank(alice); vm.expectEmit(true, true, true, true); emit ISavingCircles.CircleDecommissioned(baseCircleId); - savingCircles.decommissionCircle(baseCircleId); + savingCircles.decommission(baseCircleId); + // Verify circle was deleted vm.expectRevert(abi.encodeWithSelector(ISavingCircles.NotCommissioned.selector)); savingCircles.circle(baseCircleId); + + // Verify alice got her deposit back + assertEq(token.balanceOf(alice), DEPOSIT_AMOUNT); } - function test_AddCircleWhenCircleNameAlreadyExists() external { + function test_CreateWhenCircleNameAlreadyExists() external { vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.AlreadyExists.selector)); - savingCircles.addCircle(baseCircle); + savingCircles.create(baseCircle); } - function test_AddCircleWhenTokenIsNotWhitelisted() external { + function test_CreateWhenTokenIsNotWhitelisted() external { address _notAllowedToken = makeAddr('notAllowedToken'); ISavingCircles.Circle memory _invalidCircle = baseCircle; @@ -313,30 +338,30 @@ contract SavingCirclesUnit is Test { vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.InvalidCircle.selector)); - savingCircles.addCircle(_invalidCircle); + savingCircles.create(_invalidCircle); } - function test_AddCircleWhenIntervalIsZero() external { + function test_CreateWhenIntervalIsZero() external { ISavingCircles.Circle memory _invalidCircle = baseCircle; _invalidCircle.name = 'Invalid Circle'; _invalidCircle.depositInterval = 0; vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.InvalidCircle.selector)); - savingCircles.addCircle(_invalidCircle); + savingCircles.create(_invalidCircle); } - function test_AddCircleWhenDepositAmountIsZero() external { + function test_CreateWhenDepositAmountIsZero() external { ISavingCircles.Circle memory _invalidCircle = baseCircle; _invalidCircle.name = 'Invalid Circle'; _invalidCircle.depositAmount = 0; vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.InvalidCircle.selector)); - savingCircles.addCircle(_invalidCircle); + savingCircles.create(_invalidCircle); } - function test_AddCircleWhenMembersCountIsLessThanTwo() external { + function test_CreateWhenMembersCountIsLessThanTwo() external { address[] memory _oneMember = new address[](1); _oneMember[0] = alice; @@ -346,6 +371,6 @@ contract SavingCirclesUnit is Test { vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(ISavingCircles.InvalidCircle.selector)); - savingCircles.addCircle(_invalidCircle); + savingCircles.create(_invalidCircle); } } diff --git a/test/unit/SavingCirclesUnit.tree b/test/unit/SavingCirclesUnit.tree index 96bd838..81be96c 100644 --- a/test/unit/SavingCirclesUnit.tree +++ b/test/unit/SavingCirclesUnit.tree @@ -12,7 +12,7 @@ SavingCirclesUnit::DenylistToken ├── it denylists the token └── it emits token deniedlisted event -SavingCirclesUnit::addCircle +SavingCirclesUnit::create ├── when circle name already exists │ └── it reverts ├── when token is not whitelisted