From 1a7a0a6d295d59674016833726f5328b657fabbe Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 21 Oct 2022 12:27:32 +0200 Subject: [PATCH 1/2] RecurringPayments: implementation and integration test --- src/budget/modules/RecurringPayments.sol | 131 ++++++++++++++++++ .../test/FirmFactoryIntegrationTest.t.sol | 64 +++++++++ 2 files changed, 195 insertions(+) create mode 100644 src/budget/modules/RecurringPayments.sol diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol new file mode 100644 index 0000000..37c9a6e --- /dev/null +++ b/src/budget/modules/RecurringPayments.sol @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.16; + +import {BudgetModule} from "./BudgetModule.sol"; + +uint256 constant PAYMENTS_LENGTH_INDEX = 0; + +contract RecurringPayments is BudgetModule { + string public constant moduleId = "org.firm.budget.recurring"; + uint256 public constant moduleVersion = 1; + + struct RecurringPayment { + bool disabled; + address to; + uint256 amount; + } + + struct AllowancePayments { + mapping(uint40 => RecurringPayment) paymentData; + // tighly packed fixed array as on execution, the next execution time + // is updated, resulting in less slots being touched + // index 0 acts as the length for how many payments there are + uint40[2 ** 40] nextExecutionTime; + } + + mapping(uint256 => AllowancePayments) payments; + + event RecurringPaymentCreated( + uint256 indexed allowanceId, uint40 indexed paymentId, address indexed to, uint256 amount + ); + event RecurringPaymentExecuted( + uint256 indexed allowanceId, uint40 indexed paymentId, uint64 nextExecutionTime, address actor + ); + event RecurringPaymentsExecuted( + uint256 indexed allowanceId, uint40[] paymentIds, uint64 nextExecutionTime, address actor + ); + + error ZeroAmount(); + error UnexistentPayment(uint256 allowanceId, uint256 paymentId); + error PaymentDisabled(uint256 allowanceId, uint256 paymentId); + error AlreadyExecutedForPeriod(uint256 allowanceId, uint256 paymentId, uint40 nextExecutionTime); + + // Protected so only spenders from the parent allowance to the one recurring payments can spend can add payments + function addPayment(uint256 allowanceId, address to, uint256 amount) + external + onlyAllowanceAdmin(allowanceId) + returns (uint40 paymentId) + { + AllowancePayments storage allowancePayments = payments[allowanceId]; + + if (amount == 0) { + revert ZeroAmount(); + } + + unchecked { + paymentId = ++allowancePayments.nextExecutionTime[PAYMENTS_LENGTH_INDEX]; + } + allowancePayments.paymentData[paymentId] = RecurringPayment({disabled: false, to: to, amount: amount}); + + emit RecurringPaymentCreated(allowanceId, paymentId, to, amount); + } + + // Unprotected + function executePayment(uint256 allowanceId, uint40 paymentId) external returns (uint40 nextExecutionTime) { + RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; + uint40[2 ** 40] storage nextExecutionTimes = payments[allowanceId].nextExecutionTime; + + bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > nextExecutionTimes[PAYMENTS_LENGTH_INDEX]; + if (badPaymentId) { + revert UnexistentPayment(allowanceId, paymentId); + } + + if (payment.disabled) { + revert PaymentDisabled(allowanceId, paymentId); + } + + if (uint40(block.timestamp) < nextExecutionTimes[paymentId]) { + revert AlreadyExecutedForPeriod(allowanceId, paymentId, nextExecutionTimes[paymentId]); + } + + nextExecutionTimes[paymentId] = type(uint40).max; // reentrancy lock + nextExecutionTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + nextExecutionTimes[paymentId] = nextExecutionTime; + + emit RecurringPaymentExecuted(allowanceId, paymentId, nextExecutionTime, _msgSender()); + } + + // Unprotected + function executePayments(uint256 allowanceId, uint40[] calldata paymentIds) + external + returns (uint40 nextExecutionTime) + { + uint40[2 ** 40] storage nextExecutionTimes = payments[allowanceId].nextExecutionTime; + + uint256[] memory amounts = new uint256[](paymentIds.length); + address[] memory tos = new address[](paymentIds.length); + + uint40 paymentsLength = nextExecutionTimes[PAYMENTS_LENGTH_INDEX]; + for (uint256 i = 0; i < paymentIds.length; i++) { + uint40 paymentId = paymentIds[i]; + RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; + + bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > paymentsLength; + if (badPaymentId) { + revert UnexistentPayment(allowanceId, paymentId); + } + + if (payment.disabled) { + revert PaymentDisabled(allowanceId, paymentId); + } + + if (uint40(block.timestamp) < nextExecutionTimes[paymentId]) { + revert AlreadyExecutedForPeriod(allowanceId, paymentId, nextExecutionTimes[paymentId]); + } + + tos[i] = payment.to; + amounts[i] = payment.amount; + + // set reentrancy lock for paymentId + nextExecutionTimes[paymentId] = type(uint40).max; + } + + nextExecutionTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); + + for (uint256 i = 0; i < paymentIds.length; i++) { + nextExecutionTimes[paymentIds[i]] = nextExecutionTime; + } + + emit RecurringPaymentsExecuted(allowanceId, paymentIds, nextExecutionTime, _msgSender()); + } +} diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index 97bc494..3f20e41 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -14,6 +14,7 @@ import {Budget, TimeShiftLib, NO_PARENT_ID} from "../../budget/Budget.sol"; import {TimeShift} from "../../budget/TimeShiftLib.sol"; import {Roles, IRoles, IAvatar, ONLY_ROOT_ROLE, ROOT_ROLE_ID} from "../../roles/Roles.sol"; import {FirmRelayer} from "../../metatx/FirmRelayer.sol"; +import {RecurringPayments, BudgetModule} from "../../budget/modules/RecurringPayments.sol"; import {SafeEnums} from "../../bases/IZodiacModule.sol"; import {BokkyPooBahsDateTimeLibrary as DateTimeLib} from "datetime/BokkyPooBahsDateTimeLibrary.sol"; @@ -26,6 +27,8 @@ contract FirmFactoryIntegrationTest is FirmTest { FirmRelayer relayer; ERC20Token token; + RecurringPayments immutable recurringPaymentsImpl = new RecurringPayments(); + function setUp() public { token = new ERC20Token(); @@ -119,6 +122,67 @@ contract FirmFactoryIntegrationTest is FirmTest { assertEq(token.balanceOf(receiver), 15); } + function testPaymentsFromBudgetModules() public { + (GnosisSafe safe, Budget budget, Roles roles) = createFirm(address(this)); + token.mint(address(safe), 100); + + address spender = account("spender"); + address receiver = account("receiver"); + + vm.startPrank(address(safe)); + uint8 roleId = roles.createRole(ONLY_ROOT_ROLE, "Executive"); + roles.setRole(spender, roleId, true); + + vm.warp(DateTimeLib.timestampFromDateTime(2022, 1, 1, 0, 0, 0)); + uint256 allowanceId = budget.createAllowance( + NO_PARENT_ID, roleFlag(roleId), address(token), 6, TimeShift(TimeShiftLib.TimeUnit.Yearly, 0).encode(), "" + ); + vm.stopPrank(); + + bytes memory initRecurringPayments = abi.encodeCall(BudgetModule.initialize, (budget, address(0))); + RecurringPayments recurringPayments = RecurringPayments( + factory.moduleFactory().deployUpgradeableModule(recurringPaymentsImpl, initRecurringPayments, 1) + ); + + vm.startPrank(spender); + uint256 recurringAllowanceId = budget.createAllowance( + allowanceId, + address(recurringPayments), + address(token), + 3, + TimeShift(TimeShiftLib.TimeUnit.Monthly, 0).encode(), + "" + ); + uint40[] memory paymentIds = new uint40[](2); + paymentIds[0] = recurringPayments.addPayment(recurringAllowanceId, receiver, 1); + paymentIds[1] = recurringPayments.addPayment(recurringAllowanceId, receiver, 2); + vm.stopPrank(); + + recurringPayments.executePayments(recurringAllowanceId, paymentIds); + + // almost next month, revert bc of recurring execution too early + vm.warp(DateTimeLib.timestampFromDateTime(2022, 1, 31, 23, 59, 59)); + vm.expectRevert( + abi.encodeWithSelector( + RecurringPayments.AlreadyExecutedForPeriod.selector, + recurringAllowanceId, + paymentIds[0], + DateTimeLib.timestampFromDateTime(2022, 2, 1, 0, 0, 0) + ) + ); + recurringPayments.executePayment(recurringAllowanceId, paymentIds[0]); + + // next month + vm.warp(DateTimeLib.timestampFromDateTime(2022, 2, 1, 0, 0, 0)); + recurringPayments.executePayment(recurringAllowanceId, paymentIds[0]); + recurringPayments.executePayment(recurringAllowanceId, paymentIds[1]); + + // next month, revert bc top-level allowance is out of budget + vm.warp(DateTimeLib.timestampFromDateTime(2022, 3, 1, 0, 0, 0)); + vm.expectRevert(abi.encodeWithSelector(Budget.Overbudget.selector, allowanceId, 1, 0)); + recurringPayments.executePayment(recurringAllowanceId, paymentIds[0]); + } + function testModuleUpgrades() public { (GnosisSafe safe, Budget budget,) = createFirm(address(this)); From ebc9eb91e712add2b7379d17242ce5798ec00dd8 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Mon, 24 Oct 2022 09:07:12 +0200 Subject: [PATCH 2/2] fix --- src/budget/modules/RecurringPayments.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index 37c9a6e..6b7fd42 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -79,7 +79,7 @@ contract RecurringPayments is BudgetModule { } nextExecutionTimes[paymentId] = type(uint40).max; // reentrancy lock - nextExecutionTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + nextExecutionTime = budget().executePayment(allowanceId, payment.to, payment.amount, ""); nextExecutionTimes[paymentId] = nextExecutionTime; emit RecurringPaymentExecuted(allowanceId, paymentId, nextExecutionTime, _msgSender()); @@ -120,7 +120,7 @@ contract RecurringPayments is BudgetModule { nextExecutionTimes[paymentId] = type(uint40).max; } - nextExecutionTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); + nextExecutionTime = budget().executeMultiPayment(allowanceId, tos, amounts, ""); for (uint256 i = 0; i < paymentIds.length; i++) { nextExecutionTimes[paymentIds[i]] = nextExecutionTime;