From e9eee53920d404e56005a28ac36c736abd6456f6 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Thu, 29 Sep 2022 16:48:15 +0200 Subject: [PATCH 01/13] Budget: recurrent payment module spike --- src/budget/Budget.sol | 58 +++++++++++-- src/budget/modules/RecurringPayments.sol | 104 +++++++++++++++++++++++ 2 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 src/budget/modules/RecurringPayments.sol diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index 9823b5f..b4407a1 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -11,6 +11,7 @@ import {TimeShiftLib, EncodedTimeShift} from "./TimeShiftLib.sol"; address constant NATIVE_ASSET = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE); uint256 constant NO_PARENT_ID = 0; +uint256 constant INHERITED_AMOUNT = 0; uint40 constant INHERITED_RESET_TIME = 0; address constant IMPL_INIT_ADDRESS = address(1); @@ -58,6 +59,34 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { mapping(uint256 => Allowance) public allowances; uint256 public allowancesCount; + /* + function getAllowance(uint256 allowanceId) public view returns + ( + uint256 parentId, + uint256 amount, + uint256 spent, + address token, + uint64 nextResetTime, + address spender, + EncodedTimeShift recurrency, + bool isDisabled + ) { + Allowance storage allowance = _getAllowance(allowanceId); + + parentId = allowance.parentId; + amount = allowance.amount; + token = allowance.token; + spender = allowance.spender; + recurrency = allowance.recurrency; + isDisabled = allowance.isDisabled; + + // allowance has reset, state hasn't been updated yet + if (uint64(block.timestamp) >= nextResetTime) { + + } + } + */ + event AllowanceCreated( uint256 indexed allowanceId, uint256 indexed parentAllowanceId, @@ -95,6 +124,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { error DisabledAllowance(uint256 allowanceId); error UnauthorizedNotAllowanceAdmin(uint256 allowanceId); error TokenMismatch(address patentToken, address childToken); + error ZeroAmountForTopLevelAllowance(); error ZeroAmountPayment(); error BadInput(); error BadExecutionContext(); @@ -130,6 +160,10 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { revert UnauthorizedNotAllowanceAdmin(NO_PARENT_ID); } + if (amount == INHERITED_AMOUNT) { + revert ZeroAmountForTopLevelAllowance(); + } + // For top-level allowances, recurrency needs to be set and cannot be zero // applyShift reverts with InvalidTimeShift if recurrency is unspecified // Therefore, nextResetTime is always greater than the current time @@ -236,7 +270,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { * @param amount Amount of the allowance's token being sent * @param description Description of the payment */ - function executePayment(uint256 allowanceId, address to, uint256 amount, string memory description) external { + function executePayment(uint256 allowanceId, address to, uint256 amount, string memory description) external returns (uint40 nextResetTime) { Allowance storage allowance = _getAllowance(allowanceId); if (!_isAuthorized(_msgSender(), allowance.spender)) { @@ -250,7 +284,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { address token = allowance.token; // Make sure the payment is within budget all the way up to its top-level budget - (uint40 nextResetTime,) = _checkAndUpdateAllowanceChain(allowanceId, amount); + (nextResetTime,) = _checkAndUpdateAllowanceChain(allowanceId, amount); if (!_performTransfer(token, to, amount)) { revert PaymentExecutionFailed(allowanceId, token, to, amount); @@ -271,7 +305,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { address[] calldata tos, uint256[] calldata amounts, string memory description - ) external { + ) external returns (uint40 nextResetTime) { Allowance storage allowance = _getAllowance(allowanceId); if (!_isAuthorized(_msgSender(), allowance.spender)) { @@ -292,7 +326,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { } } - (uint40 nextResetTime,) = _checkAndUpdateAllowanceChain(allowanceId, totalAmount); + (nextResetTime,) = _checkAndUpdateAllowanceChain(allowanceId, totalAmount); address token = allowance.token; if (!_performMultiTransfer(token, tos, amounts)) { @@ -375,6 +409,10 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { return allowances[allowanceId]; } + function isAdminOnAllowance(uint256 allowanceId, address actor) public view returns (bool) { + return _isAdminOnAllowance(_getAllowance(allowanceId), actor); + } + function _isAdminOnAllowance(Allowance storage allowance, address actor) internal view returns (bool) { // Changes to the allowance state can be done by the same entity that could // create that allowance in the first place @@ -415,11 +453,13 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { } } - uint256 spentAfterPayment = amount + (allowanceResets ? 0 : allowance.spent); - if (spentAfterPayment > allowance.amount) { - revert Overbudget(allowanceId, amount, allowance.amount - allowance.spent); - } + if (allowance.amount != INHERITED_AMOUNT) { + uint256 spentAfterPayment = amount + (allowanceResets ? 0 : allowance.spent); + if (spentAfterPayment > allowance.amount) { + revert Overbudget(allowanceId, amount, allowance.amount - allowance.spent); + } - allowance.spent = spentAfterPayment; + allowance.spent = spentAfterPayment; + } } } diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol new file mode 100644 index 0000000..fc895d7 --- /dev/null +++ b/src/budget/modules/RecurringPayments.sol @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.16; + +import {Budget} from "../Budget.sol"; +import {FirmBase, IAvatar} from "../../bases/FirmBase.sol"; + +abstract contract BudgetModule is FirmBase { + Budget public budget; // TODO: Unstructured storage + + function initialize(Budget budget_, address trustedForwarder_) public { + __init_firmBase(budget_.safe(), trustedForwarder_); + budget = budget_; + } + + error UnauthorizedNotAllowanceAdmin(uint256 allowanceId, address actor); + + modifier onlyAllowanceAdmin(uint256 allowanceId) { + address actor = _msgSender(); + if (!budget.isAdminOnAllowance(allowanceId, actor)) { + revert UnauthorizedNotAllowanceAdmin(allowanceId, actor); + } + + _; + } +} + +contract RecurringPayments is BudgetModule { + string public constant moduleId = "org.firm.budget.recurring"; + uint256 public constant moduleVersion = 1; + + struct RecurringPayment { + bool enabled; + address to; + uint256 amount; + } + + struct AllowancePayments { + mapping (uint256 => RecurringPayment) paymentData; + mapping (uint256 => uint64[4]) nextExecutionTime; // tightly stored as an optimization + uint256 paymentsCount; + } + + mapping (uint256 => AllowancePayments) payments; + + // 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 (uint256 paymentId) { + AllowancePayments storage allowancePayments = payments[allowanceId]; + + unchecked { + paymentId = ++allowancePayments.paymentsCount; + } + allowancePayments.paymentData[paymentId].to = to; + allowancePayments.paymentData[paymentId].amount = amount; + } + + // Unprotected + function executePayment(uint256 allowanceId, uint256 paymentId) external { + RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; + + uint256 id1 = paymentId / 4; + uint256 id2 = paymentId % 4; + require(payment.enabled); + require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); + + // reentrancy lock + payments[allowanceId].nextExecutionTime[id1][id2] = type(uint64).max; + + uint64 nextResetTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + + payments[allowanceId].nextExecutionTime[id1][id2] = nextResetTime; + } + + function executeManyPayments(uint256 allowanceId, uint256[] calldata paymentIds) external { + uint256[] memory amounts = new uint256[](paymentIds.length); + address[] memory tos = new address[](paymentIds.length); + + for (uint256 i = 0; i < paymentIds.length; i++) { + uint256 paymentId = paymentIds[i]; + RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; + + uint256 id1 = paymentId / 4; + uint256 id2 = paymentId % 4; + + require(payment.enabled); + require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); + + tos[i] = payment.to; + amounts[i] = payment.amount; + + // set reentrancy lock for paymentId + payments[allowanceId].nextExecutionTime[id1][id2] = type(uint64).max; + } + + uint64 nextResetTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); + + for (uint256 i = 0; i < paymentIds.length; i++) { + uint256 paymentId = paymentIds[i]; + uint256 id1 = paymentId / 4; + uint256 id2 = paymentId % 4; + + payments[allowanceId].nextExecutionTime[id1][id2] = nextResetTime; + } + } +} From 97d75a4959d9696725d5630e649145788fa12739 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Thu, 29 Sep 2022 16:49:32 +0200 Subject: [PATCH 02/13] Delete dead code --- src/budget/Budget.sol | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index b4407a1..a2fb6f3 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -59,34 +59,6 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { mapping(uint256 => Allowance) public allowances; uint256 public allowancesCount; - /* - function getAllowance(uint256 allowanceId) public view returns - ( - uint256 parentId, - uint256 amount, - uint256 spent, - address token, - uint64 nextResetTime, - address spender, - EncodedTimeShift recurrency, - bool isDisabled - ) { - Allowance storage allowance = _getAllowance(allowanceId); - - parentId = allowance.parentId; - amount = allowance.amount; - token = allowance.token; - spender = allowance.spender; - recurrency = allowance.recurrency; - isDisabled = allowance.isDisabled; - - // allowance has reset, state hasn't been updated yet - if (uint64(block.timestamp) >= nextResetTime) { - - } - } - */ - event AllowanceCreated( uint256 indexed allowanceId, uint256 indexed parentAllowanceId, From f3b948735986f89b1ea870b463373c047f3b6e9f Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 30 Sep 2022 14:09:32 +0200 Subject: [PATCH 03/13] Split BudgetModule --- src/budget/Budget.sol | 2 + src/budget/modules/BudgetModule.sol | 25 ++++++++ src/budget/modules/RecurringPayments.sol | 43 +++++--------- .../test/FirmFactoryIntegrationTest.t.sol | 58 +++++++++++++++++++ 4 files changed, 98 insertions(+), 30 deletions(-) create mode 100644 src/budget/modules/BudgetModule.sol diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index a2fb6f3..b8ba1aa 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -206,6 +206,8 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { */ function setAllowanceAmount(uint256 allowanceId, uint256 amount) external { Allowance storage allowance = _getAllowanceAndValidateAdmin(allowanceId); + + // TODO: Not let setting 0 if allowance is top level allowance.amount = amount; emit AllowanceAmountChanged(allowanceId, amount); } diff --git a/src/budget/modules/BudgetModule.sol b/src/budget/modules/BudgetModule.sol new file mode 100644 index 0000000..60ddbe4 --- /dev/null +++ b/src/budget/modules/BudgetModule.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.16; + +import {Budget} from "../Budget.sol"; +import {FirmBase, IAvatar} from "../../bases/FirmBase.sol"; + +abstract contract BudgetModule is FirmBase { + Budget public budget; // TODO: Unstructured storage + + function initialize(Budget budget_, address trustedForwarder_) public { + __init_firmBase(budget_.safe(), trustedForwarder_); + budget = budget_; + } + + error UnauthorizedNotAllowanceAdmin(uint256 allowanceId, address actor); + + modifier onlyAllowanceAdmin(uint256 allowanceId) { + address actor = _msgSender(); + if (!budget.isAdminOnAllowance(allowanceId, actor)) { + revert UnauthorizedNotAllowanceAdmin(allowanceId, actor); + } + + _; + } +} diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index fc895d7..58283ae 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -1,53 +1,36 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.16; -import {Budget} from "../Budget.sol"; -import {FirmBase, IAvatar} from "../../bases/FirmBase.sol"; - -abstract contract BudgetModule is FirmBase { - Budget public budget; // TODO: Unstructured storage - - function initialize(Budget budget_, address trustedForwarder_) public { - __init_firmBase(budget_.safe(), trustedForwarder_); - budget = budget_; - } - - error UnauthorizedNotAllowanceAdmin(uint256 allowanceId, address actor); - - modifier onlyAllowanceAdmin(uint256 allowanceId) { - address actor = _msgSender(); - if (!budget.isAdminOnAllowance(allowanceId, actor)) { - revert UnauthorizedNotAllowanceAdmin(allowanceId, actor); - } - - _; - } -} +import {BudgetModule} from "./BudgetModule.sol"; contract RecurringPayments is BudgetModule { string public constant moduleId = "org.firm.budget.recurring"; uint256 public constant moduleVersion = 1; struct RecurringPayment { - bool enabled; + bool disabled; address to; uint256 amount; } struct AllowancePayments { - mapping (uint256 => RecurringPayment) paymentData; - mapping (uint256 => uint64[4]) nextExecutionTime; // tightly stored as an optimization + mapping(uint256 => RecurringPayment) paymentData; + mapping(uint256 => uint64[4]) nextExecutionTime; // tightly stored as an optimization uint256 paymentsCount; } - mapping (uint256 => AllowancePayments) payments; + mapping(uint256 => AllowancePayments) payments; // 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 (uint256 paymentId) { + function addPayment(uint256 allowanceId, address to, uint256 amount) + external + onlyAllowanceAdmin(allowanceId) + returns (uint256 paymentId) + { AllowancePayments storage allowancePayments = payments[allowanceId]; unchecked { - paymentId = ++allowancePayments.paymentsCount; + paymentId = allowancePayments.paymentsCount++; } allowancePayments.paymentData[paymentId].to = to; allowancePayments.paymentData[paymentId].amount = amount; @@ -59,7 +42,7 @@ contract RecurringPayments is BudgetModule { uint256 id1 = paymentId / 4; uint256 id2 = paymentId % 4; - require(payment.enabled); + require(!payment.disabled); require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); // reentrancy lock @@ -81,7 +64,7 @@ contract RecurringPayments is BudgetModule { uint256 id1 = paymentId / 4; uint256 id2 = paymentId % 4; - require(payment.enabled); + require(!payment.disabled); require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); tos[i] = payment.to; diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index cdabf21..04f731a 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -14,7 +14,9 @@ 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"; import {LocalDeploy} from "../../../scripts/LocalDeploy.sol"; @@ -25,6 +27,8 @@ contract FirmFactoryIntegrationTest is FirmTest { FirmRelayer relayer; ERC20Token token; + RecurringPayments immutable recurringPaymentsImpl = new RecurringPayments(); + function setUp() public { token = new ERC20Token(); @@ -118,6 +122,60 @@ 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(), + "" + ); + uint256[] memory paymentIds = new uint256[](2); + paymentIds[0] = recurringPayments.addPayment(recurringAllowanceId, receiver, 1); + paymentIds[1] = recurringPayments.addPayment(recurringAllowanceId, receiver, 2); + vm.stopPrank(); + + recurringPayments.executeManyPayments(recurringAllowanceId, paymentIds); + + // almost next month, revert bc of recurring execution too early + vm.warp(DateTimeLib.timestampFromDateTime(2022, 1, 31, 23, 59, 59)); + vm.expectRevert(bytes("")); + 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 0406f40c57ce0d058129cb3414ed77bfd13d7d61 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Sat, 1 Oct 2022 18:16:05 +0200 Subject: [PATCH 04/13] Optimize --- src/budget/Budget.sol | 5 +- src/budget/modules/RecurringPayments.sol | 57 +++++++++++-------- .../test/FirmFactoryIntegrationTest.t.sol | 2 +- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index b8ba1aa..f643a4d 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -244,7 +244,10 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { * @param amount Amount of the allowance's token being sent * @param description Description of the payment */ - function executePayment(uint256 allowanceId, address to, uint256 amount, string memory description) external returns (uint40 nextResetTime) { + function executePayment(uint256 allowanceId, address to, uint256 amount, string memory description) + external + returns (uint40 nextResetTime) + { Allowance storage allowance = _getAllowance(allowanceId); if (!_isAuthorized(_msgSender(), allowance.spender)) { diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index 58283ae..ae36610 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -15,73 +15,84 @@ contract RecurringPayments is BudgetModule { struct AllowancePayments { mapping(uint256 => RecurringPayment) paymentData; - mapping(uint256 => uint64[4]) nextExecutionTime; // tightly stored as an optimization - uint256 paymentsCount; + // 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; } + uint256 internal constant PAYMENTS_LENGTH_INDEX = 0; + mapping(uint256 => AllowancePayments) payments; // 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 (uint256 paymentId) + returns (uint40 paymentId) { AllowancePayments storage allowancePayments = payments[allowanceId]; unchecked { - paymentId = allowancePayments.paymentsCount++; + paymentId = ++allowancePayments.nextExecutionTime[PAYMENTS_LENGTH_INDEX]; } - allowancePayments.paymentData[paymentId].to = to; - allowancePayments.paymentData[paymentId].amount = amount; + allowancePayments.paymentData[paymentId] = RecurringPayment({disabled: false, to: to, amount: amount}); } + + error UnexistentPayment(uint256 allowanceId, uint256 paymentId); // Unprotected function executePayment(uint256 allowanceId, uint256 paymentId) external { RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; + uint40[2 ** 40] storage nextExecutionTime = payments[allowanceId].nextExecutionTime; + + bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > nextExecutionTime[PAYMENTS_LENGTH_INDEX]; + if (badPaymentId) { + revert UnexistentPayment(allowanceId, paymentId); + } - uint256 id1 = paymentId / 4; - uint256 id2 = paymentId % 4; require(!payment.disabled); - require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); + require(uint40(block.timestamp) >= nextExecutionTime[paymentId]); // reentrancy lock - payments[allowanceId].nextExecutionTime[id1][id2] = type(uint64).max; + nextExecutionTime[paymentId] = type(uint40).max; - uint64 nextResetTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + uint40 nextResetTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); - payments[allowanceId].nextExecutionTime[id1][id2] = nextResetTime; + nextExecutionTime[paymentId] = nextResetTime; } - function executeManyPayments(uint256 allowanceId, uint256[] calldata paymentIds) external { + // Unprotected + function executeManyPayments(uint256 allowanceId, uint40[] calldata paymentIds) external { + uint40[2 ** 40] storage nextExecutionTime = payments[allowanceId].nextExecutionTime; + uint256[] memory amounts = new uint256[](paymentIds.length); address[] memory tos = new address[](paymentIds.length); + uint40 paymentsLength = nextExecutionTime[PAYMENTS_LENGTH_INDEX]; for (uint256 i = 0; i < paymentIds.length; i++) { uint256 paymentId = paymentIds[i]; RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; - uint256 id1 = paymentId / 4; - uint256 id2 = paymentId % 4; + bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > paymentsLength; + if (badPaymentId) { + revert UnexistentPayment(allowanceId, paymentId); + } require(!payment.disabled); - require(uint64(block.timestamp) >= payments[allowanceId].nextExecutionTime[id1][id2]); + require(uint40(block.timestamp) >= nextExecutionTime[paymentId]); tos[i] = payment.to; amounts[i] = payment.amount; // set reentrancy lock for paymentId - payments[allowanceId].nextExecutionTime[id1][id2] = type(uint64).max; + nextExecutionTime[paymentId] = type(uint40).max; } - uint64 nextResetTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); + uint40 nextResetTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); for (uint256 i = 0; i < paymentIds.length; i++) { - uint256 paymentId = paymentIds[i]; - uint256 id1 = paymentId / 4; - uint256 id2 = paymentId % 4; - - payments[allowanceId].nextExecutionTime[id1][id2] = nextResetTime; + nextExecutionTime[paymentIds[i]] = nextResetTime; } } } diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index 04f731a..735762a 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -153,7 +153,7 @@ contract FirmFactoryIntegrationTest is FirmTest { TimeShift(TimeShiftLib.TimeUnit.Monthly, 0).encode(), "" ); - uint256[] memory paymentIds = new uint256[](2); + uint40[] memory paymentIds = new uint40[](2); paymentIds[0] = recurringPayments.addPayment(recurringAllowanceId, receiver, 1); paymentIds[1] = recurringPayments.addPayment(recurringAllowanceId, receiver, 2); vm.stopPrank(); From 65cf20caf3162dcd7a8e92a409aee9dda178ad7c Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Sat, 1 Oct 2022 18:23:05 +0200 Subject: [PATCH 05/13] Budget: disallow changing amount of top-level allowance to 0 and explanation --- src/budget/Budget.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index f643a4d..b1b196d 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -132,6 +132,9 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { revert UnauthorizedNotAllowanceAdmin(NO_PARENT_ID); } + // We don't allow setting amount 0 on top-level allowances as clients + // could support setting 0 as the amount for the allowance and that + // will create an allowance that allows completely wiping the safe (for the token) if (amount == INHERITED_AMOUNT) { revert ZeroAmountForTopLevelAllowance(); } @@ -207,7 +210,10 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { function setAllowanceAmount(uint256 allowanceId, uint256 amount) external { Allowance storage allowance = _getAllowanceAndValidateAdmin(allowanceId); - // TODO: Not let setting 0 if allowance is top level + if (amount == INHERITED_AMOUNT && allowance.parentId == NO_PARENT_ID) { + revert ZeroAmountForTopLevelAllowance(); + } + allowance.amount = amount; emit AllowanceAmountChanged(allowanceId, amount); } From 0116bd199d665a4b458b8b4a9b53bbc7c329c8e2 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Sat, 1 Oct 2022 18:23:30 +0200 Subject: [PATCH 06/13] RecurringPayments: disallow creating payments with 0 amount --- src/budget/modules/RecurringPayments.sol | 12 ++++++++---- src/factory/test/FirmFactoryIntegrationTest.t.sol | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index ae36610..ebeacd1 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -3,6 +3,8 @@ pragma solidity 0.8.16; import {BudgetModule} from "./BudgetModule.sol"; +uint256 internal constant PAYMENTS_LENGTH_INDEX = 0; + contract RecurringPayments is BudgetModule { string public constant moduleId = "org.firm.budget.recurring"; uint256 public constant moduleVersion = 1; @@ -33,13 +35,15 @@ contract RecurringPayments is BudgetModule { { 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}); } - - error UnexistentPayment(uint256 allowanceId, uint256 paymentId); // Unprotected function executePayment(uint256 allowanceId, uint256 paymentId) external { @@ -65,7 +69,7 @@ contract RecurringPayments is BudgetModule { // Unprotected function executeManyPayments(uint256 allowanceId, uint40[] calldata paymentIds) external { uint40[2 ** 40] storage nextExecutionTime = payments[allowanceId].nextExecutionTime; - + uint256[] memory amounts = new uint256[](paymentIds.length); address[] memory tos = new address[](paymentIds.length); @@ -77,7 +81,7 @@ contract RecurringPayments is BudgetModule { bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > paymentsLength; if (badPaymentId) { revert UnexistentPayment(allowanceId, paymentId); - } + } require(!payment.disabled); require(uint40(block.timestamp) >= nextExecutionTime[paymentId]); diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index 735762a..4ee44ee 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -158,7 +158,7 @@ contract FirmFactoryIntegrationTest is FirmTest { paymentIds[1] = recurringPayments.addPayment(recurringAllowanceId, receiver, 2); vm.stopPrank(); - recurringPayments.executeManyPayments(recurringAllowanceId, paymentIds); + recurringPayments.executeManyPayments(recurringAllowanceId, paymentIds); // almost next month, revert bc of recurring execution too early vm.warp(DateTimeLib.timestampFromDateTime(2022, 1, 31, 23, 59, 59)); From 4e4261f070a5f62dab34ec24d60b03894cefdaed Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Sat, 1 Oct 2022 18:38:33 +0200 Subject: [PATCH 07/13] RecurringPayments: events and errors --- src/budget/modules/RecurringPayments.sol | 64 ++++++++++++------- .../test/FirmFactoryIntegrationTest.t.sol | 4 +- 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index ebeacd1..9357342 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.16; import {BudgetModule} from "./BudgetModule.sol"; -uint256 internal constant PAYMENTS_LENGTH_INDEX = 0; +uint256 constant PAYMENTS_LENGTH_INDEX = 0; contract RecurringPayments is BudgetModule { string public constant moduleId = "org.firm.budget.recurring"; @@ -16,17 +16,24 @@ contract RecurringPayments is BudgetModule { } struct AllowancePayments { - mapping(uint256 => RecurringPayment) paymentData; + 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; } - uint256 internal constant PAYMENTS_LENGTH_INDEX = 0; - 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 @@ -43,39 +50,45 @@ contract RecurringPayments is BudgetModule { 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, uint256 paymentId) external { + function executePayment(uint256 allowanceId, uint40 paymentId) external returns (uint40 nextExecutionTime) { RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; - uint40[2 ** 40] storage nextExecutionTime = payments[allowanceId].nextExecutionTime; + uint40[2 ** 40] storage nextExecutionTimes = payments[allowanceId].nextExecutionTime; - bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > nextExecutionTime[PAYMENTS_LENGTH_INDEX]; + bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > nextExecutionTimes[PAYMENTS_LENGTH_INDEX]; if (badPaymentId) { revert UnexistentPayment(allowanceId, paymentId); } - require(!payment.disabled); - require(uint40(block.timestamp) >= nextExecutionTime[paymentId]); + if (payment.disabled) { + revert PaymentDisabled(allowanceId, paymentId); + } - // reentrancy lock - nextExecutionTime[paymentId] = type(uint40).max; + if (uint40(block.timestamp) < nextExecutionTimes[paymentId]) { + revert AlreadyExecutedForPeriod(allowanceId, paymentId, nextExecutionTimes[paymentId]); + } - uint40 nextResetTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + nextExecutionTimes[paymentId] = type(uint40).max; // reentrancy lock + nextExecutionTime = budget.executePayment(allowanceId, payment.to, payment.amount, ""); + nextExecutionTimes[paymentId] = nextExecutionTime; - nextExecutionTime[paymentId] = nextResetTime; + emit RecurringPaymentExecuted(allowanceId, paymentId, nextExecutionTime, _msgSender()); } // Unprotected - function executeManyPayments(uint256 allowanceId, uint40[] calldata paymentIds) external { - uint40[2 ** 40] storage nextExecutionTime = payments[allowanceId].nextExecutionTime; + 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 = nextExecutionTime[PAYMENTS_LENGTH_INDEX]; + uint40 paymentsLength = nextExecutionTimes[PAYMENTS_LENGTH_INDEX]; for (uint256 i = 0; i < paymentIds.length; i++) { - uint256 paymentId = paymentIds[i]; + uint40 paymentId = paymentIds[i]; RecurringPayment storage payment = payments[allowanceId].paymentData[paymentId]; bool badPaymentId = paymentId == PAYMENTS_LENGTH_INDEX || paymentId > paymentsLength; @@ -83,20 +96,27 @@ contract RecurringPayments is BudgetModule { revert UnexistentPayment(allowanceId, paymentId); } - require(!payment.disabled); - require(uint40(block.timestamp) >= nextExecutionTime[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 - nextExecutionTime[paymentId] = type(uint40).max; + nextExecutionTimes[paymentId] = type(uint40).max; } - uint40 nextResetTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); + nextExecutionTime = budget.executeMultiPayment(allowanceId, tos, amounts, ""); for (uint256 i = 0; i < paymentIds.length; i++) { - nextExecutionTime[paymentIds[i]] = nextResetTime; + 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 4ee44ee..ebd66ed 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -158,11 +158,11 @@ contract FirmFactoryIntegrationTest is FirmTest { paymentIds[1] = recurringPayments.addPayment(recurringAllowanceId, receiver, 2); vm.stopPrank(); - recurringPayments.executeManyPayments(recurringAllowanceId, paymentIds); + 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(bytes("")); + 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 From cd05c50d6aa83ea6ec5eeaa42a08fdd901736069 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 7 Oct 2022 15:11:09 +0200 Subject: [PATCH 08/13] BudgetModule: initialize on constructor --- src/budget/modules/BudgetModule.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/budget/modules/BudgetModule.sol b/src/budget/modules/BudgetModule.sol index 60ddbe4..d056c97 100644 --- a/src/budget/modules/BudgetModule.sol +++ b/src/budget/modules/BudgetModule.sol @@ -4,11 +4,19 @@ pragma solidity 0.8.16; import {Budget} from "../Budget.sol"; import {FirmBase, IAvatar} from "../../bases/FirmBase.sol"; +address constant IMPL_INIT_ADDRESS = address(1); + abstract contract BudgetModule is FirmBase { Budget public budget; // TODO: Unstructured storage + constructor() { + // Initialize with impossible values in constructor so impl base cannot be used + initialize(Budget(IMPL_INIT_ADDRESS), IMPL_INIT_ADDRESS); + } + function initialize(Budget budget_, address trustedForwarder_) public { - __init_firmBase(budget_.safe(), trustedForwarder_); + IAvatar safe = address(budget_) != IMPL_INIT_ADDRESS ? budget_.safe() : IAvatar(IMPL_INIT_ADDRESS); + __init_firmBase(safe, trustedForwarder_); budget = budget_; } From ca5d63af8055f763d219dc2029e0285d37d8a89f Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Mon, 17 Oct 2022 12:54:11 -0500 Subject: [PATCH 09/13] UpgradeableModuleProxyFactory: registry (#80) * UpgradeableModuleProxyFactory: register * FirmBase, UpgradeableModuleProxyFactory: events to enable indexing * UpgradeableModuleProxyFactory: test upgrades and tighten types everywhere * UpgradeableModuleProxyFactory: test registry * UpgradeableModuleProxyFactory: more tests --- src/bases/EIP1967Upgradeable.sol | 14 +- src/bases/ERC2771Context.sol | 1 + src/bases/FirmBase.sol | 5 + src/bases/test/EIP1967Upgradeable.t.sol | 4 +- src/budget/Budget.sol | 2 +- src/budget/modules/RecurringPayments.sol | 17 +- src/common/test/lib/FirmTest.sol | 4 +- src/factory/UpgradeableModuleProxyFactory.sol | 77 ++++++-- .../test/FirmFactoryIntegrationTest.t.sol | 13 +- .../test/UpgradeableModuleProxyFactory.t.sol | 165 ++++++++++++++---- src/factory/test/lib/TestTargets.sol | 44 +++++ .../UpgradeableModuleProxyFactoryMock.sol | 10 -- 12 files changed, 278 insertions(+), 78 deletions(-) create mode 100644 src/factory/test/lib/TestTargets.sol delete mode 100644 src/factory/test/mocks/UpgradeableModuleProxyFactoryMock.sol diff --git a/src/bases/EIP1967Upgradeable.sol b/src/bases/EIP1967Upgradeable.sol index 7f5f017..467036f 100644 --- a/src/bases/EIP1967Upgradeable.sol +++ b/src/bases/EIP1967Upgradeable.sol @@ -11,7 +11,7 @@ import {IModuleMetadata} from "./IModuleMetadata.sol"; * address must already be set in the correct slot (in our case, the proxy does on creation) */ abstract contract EIP1967Upgradeable is SafeAware { - event Upgraded(address indexed implementation, string moduleId, uint256 version); + event Upgraded(IModuleMetadata indexed implementation, string moduleId, uint256 version); // EIP1967_IMPL_SLOT = keccak256('eip1967.proxy.implementation') - 1 bytes32 internal constant EIP1967_IMPL_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; @@ -33,17 +33,15 @@ abstract contract EIP1967Upgradeable is SafeAware { * It also must conform to the IModuleMetadata interface (this is somewhat of an implicit guard against bad upgrades) * @param _newImplementation The address of the new implementation address the proxy will use */ - function upgrade(address _newImplementation) public onlySafe { + function upgrade(IModuleMetadata _newImplementation) public onlySafe { assembly { sstore(EIP1967_IMPL_SLOT, _newImplementation) } - IModuleMetadata upgradeMetadata = IModuleMetadata(_newImplementation); - - emit Upgraded(_newImplementation, upgradeMetadata.moduleId(), upgradeMetadata.moduleVersion()); + emit Upgraded(_newImplementation, _newImplementation.moduleId(), _newImplementation.moduleVersion()); } - function _implementation() internal view returns (address impl) { + function _implementation() internal view returns (IModuleMetadata impl) { assembly { impl := sload(EIP1967_IMPL_SLOT) } @@ -56,10 +54,10 @@ abstract contract EIP1967Upgradeable is SafeAware { * If we were running in impl conext, the IMPL_CONTRACT_FLAG would be stored there */ function _isForeignContext() internal view returns (bool) { - return _implementation() == address(0); + return address(_implementation()) == address(0); } function _isImplementationContext() internal view returns (bool) { - return _implementation() == IMPL_CONTRACT_FLAG; + return address(_implementation()) == IMPL_CONTRACT_FLAG; } } diff --git a/src/bases/ERC2771Context.sol b/src/bases/ERC2771Context.sol index 950eae6..62b9791 100644 --- a/src/bases/ERC2771Context.sol +++ b/src/bases/ERC2771Context.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.16; import {SafeAware} from "./SafeAware.sol"; diff --git a/src/bases/FirmBase.sol b/src/bases/FirmBase.sol index 5e9627b..93720c9 100644 --- a/src/bases/FirmBase.sol +++ b/src/bases/FirmBase.sol @@ -7,7 +7,12 @@ import {EIP1967Upgradeable} from "./EIP1967Upgradeable.sol"; import {IModuleMetadata} from "./IModuleMetadata.sol"; abstract contract FirmBase is EIP1967Upgradeable, ERC2771Context, IModuleMetadata { + event Initialized(IAvatar indexed safe, IModuleMetadata indexed implementation); + function __init_firmBase(IAvatar safe_, address trustedForwarder_) internal { + // checks-effects-interactions violated so that the init event always fires first + emit Initialized(safe_, _implementation()); + __init_setSafe(safe_); if (trustedForwarder_ != address(0)) { _setTrustedForwarder(trustedForwarder_, true); diff --git a/src/bases/test/EIP1967Upgradeable.t.sol b/src/bases/test/EIP1967Upgradeable.t.sol index 133044f..dd81169 100644 --- a/src/bases/test/EIP1967Upgradeable.t.sol +++ b/src/bases/test/EIP1967Upgradeable.t.sol @@ -33,7 +33,7 @@ contract EIP1967UpgradeableTest is BasesTest { vm.prank(address(avatar)); vm.expectEmit(true, true, false, true); emit Upgraded(address(moduleTwoImpl), "org.firm.modulemock", 0); - module.upgrade(address(moduleTwoImpl)); + module.upgrade(moduleTwoImpl); assertImplAtEIP1967Slot(address(moduleTwoImpl)); assertEq(module.foo(), MODULE_TWO_FOO); @@ -42,6 +42,6 @@ contract EIP1967UpgradeableTest is BasesTest { function testNonAvatarCannotUpgrade() public { vm.expectRevert(abi.encodeWithSelector(SafeAware.UnauthorizedNotSafe.selector)); - module.upgrade(address(moduleTwoImpl)); + module.upgrade(moduleTwoImpl); } } diff --git a/src/budget/Budget.sol b/src/budget/Budget.sol index b1b196d..6452021 100644 --- a/src/budget/Budget.sol +++ b/src/budget/Budget.sol @@ -337,7 +337,7 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { bytes memory data = abi.encodeCall(this.__safeContext_performMultiTransfer, (token, tos, amounts)); (bool callSuccess, bytes memory retData) = - execAndReturnData(_implementation(), 0, data, SafeEnums.Operation.DelegateCall); + execAndReturnData(address(_implementation()), 0, data, SafeEnums.Operation.DelegateCall); return callSuccess && retData.length == 32 && abi.decode(retData, (bool)); } diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol index 9357342..37c9a6e 100644 --- a/src/budget/modules/RecurringPayments.sol +++ b/src/budget/modules/RecurringPayments.sol @@ -25,9 +25,15 @@ contract RecurringPayments is BudgetModule { 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); + 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); @@ -80,7 +86,10 @@ contract RecurringPayments is BudgetModule { } // Unprotected - function executePayments(uint256 allowanceId, uint40[] calldata paymentIds) external returns (uint40 nextExecutionTime) { + 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); diff --git a/src/common/test/lib/FirmTest.sol b/src/common/test/lib/FirmTest.sol index fdcf90f..7bdc57f 100644 --- a/src/common/test/lib/FirmTest.sol +++ b/src/common/test/lib/FirmTest.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "../../../factory/UpgradeableModuleProxyFactory.sol"; -import {EIP1967Upgradeable} from "../../../bases/EIP1967Upgradeable.sol"; +import {FirmBase} from "../../../bases/FirmBase.sol"; contract FirmTest is Test { UpgradeableModuleProxyFactory immutable proxyFactory = new UpgradeableModuleProxyFactory(); @@ -19,7 +19,7 @@ contract FirmTest is Test { vm.label(addr, label); } - function createProxy(EIP1967Upgradeable impl, bytes memory initdata) internal returns (address proxy) { + function createProxy(FirmBase impl, bytes memory initdata) internal returns (address proxy) { proxy = proxyFactory.deployUpgradeableModule(impl, initdata, 0); vm.label(proxy, "Proxy"); } diff --git a/src/factory/UpgradeableModuleProxyFactory.sol b/src/factory/UpgradeableModuleProxyFactory.sol index fba6120..0e6bde7 100644 --- a/src/factory/UpgradeableModuleProxyFactory.sol +++ b/src/factory/UpgradeableModuleProxyFactory.sol @@ -1,42 +1,83 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.16; -import {EIP1967Upgradeable} from "../bases/EIP1967Upgradeable.sol"; +import {Ownable} from "openzeppelin/access/Ownable.sol"; +import {IModuleMetadata} from "../bases/IModuleMetadata.sol"; -contract UpgradeableModuleProxyFactory { - error TakenAddress(address proxy); +uint256 constant LATEST_VERSION = type(uint256).max; + +contract UpgradeableModuleProxyFactory is Ownable { + error ProxyAlreadyDeployedForNonce(); error FailedInitialization(); + error ModuleVersionAlreadyRegistered(); + error UnexistentModuleVersion(); - event ModuleProxyCreation(address indexed proxy, EIP1967Upgradeable indexed implementation); + event ModuleRegistered(IModuleMetadata indexed implementation, string moduleId, uint256 version); + event ModuleProxyCreation(address indexed proxy, IModuleMetadata indexed implementation); - function createUpgradeableProxy(EIP1967Upgradeable implementation, bytes32 salt) internal returns (address addr) { - // if (address(target) == address(0)) revert ZeroAddress(target); - // Removed as this is a responsibility of the caller and we shouldn't pay for the check on each proxy creation - bytes memory initcode = abi.encodePacked( - hex"73", - implementation, - hex"7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc55603b8060403d393df3363d3d3760393d3d3d3d3d363d7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc545af4913d913e3d9257fd5bf3" - ); + mapping(string => mapping(uint256 => IModuleMetadata)) internal modules; + mapping(string => uint256) public latestModuleVersion; - assembly { - addr := create2(0, add(initcode, 0x20), mload(initcode), salt) + function register(IModuleMetadata implementation) external onlyOwner { + string memory moduleId = implementation.moduleId(); + uint256 version = implementation.moduleVersion(); + + if (address(modules[moduleId][version]) != address(0)) { + revert ModuleVersionAlreadyRegistered(); } - if (addr == address(0)) { - revert TakenAddress(addr); + modules[moduleId][version] = implementation; + + if (version > latestModuleVersion[moduleId]) { + latestModuleVersion[moduleId] = version; + } + + emit ModuleRegistered(implementation, moduleId, version); + } + + function getImplementation(string memory moduleId, uint256 version) public view returns (IModuleMetadata implementation) { + if (version == LATEST_VERSION) { + version = latestModuleVersion[moduleId]; + } + implementation = modules[moduleId][version]; + if (address(implementation) == address(0)) { + revert UnexistentModuleVersion(); } } - function deployUpgradeableModule(EIP1967Upgradeable implementation, bytes memory initializer, uint256 salt) + function deployUpgradeableModule(string memory moduleId, uint256 version, bytes memory initializer, uint256 salt) public returns (address proxy) { - proxy = createUpgradeableProxy(implementation, keccak256(abi.encodePacked(keccak256(initializer), salt))); + return deployUpgradeableModule(getImplementation(moduleId, version), initializer, salt); + } + + function deployUpgradeableModule(IModuleMetadata implementation, bytes memory initializer, uint256 salt) + public + returns (address proxy) + { + proxy = createProxy(implementation, keccak256(abi.encodePacked(keccak256(initializer), salt))); (bool success,) = proxy.call(initializer); if (!success) { revert FailedInitialization(); } + } + + function createProxy(IModuleMetadata implementation, bytes32 salt) internal returns (address proxy) { + bytes memory initcode = abi.encodePacked( + hex"73", + implementation, + hex"7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc55603b8060403d393df3363d3d3760393d3d3d3d3d363d7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc545af4913d913e3d9257fd5bf3" + ); + + assembly { + proxy := create2(0, add(initcode, 0x20), mload(initcode), salt) + } + + if (proxy == address(0)) { + revert ProxyAlreadyDeployedForNonce(); + } emit ModuleProxyCreation(proxy, implementation); } diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index ebd66ed..c2bba83 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -162,7 +162,14 @@ contract FirmFactoryIntegrationTest is FirmTest { // 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))); + 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 @@ -179,9 +186,9 @@ contract FirmFactoryIntegrationTest is FirmTest { function testModuleUpgrades() public { (GnosisSafe safe, Budget budget,) = createFirm(address(this)); - address moduleMockImpl = address(new ModuleMock(1)); + ModuleMock newImpl = new ModuleMock(1); vm.prank(address(safe)); - budget.upgrade(moduleMockImpl); + budget.upgrade(newImpl); assertEq(ModuleMock(address(budget)).foo(), 1); } diff --git a/src/factory/test/UpgradeableModuleProxyFactory.t.sol b/src/factory/test/UpgradeableModuleProxyFactory.t.sol index 8e8e115..6d37132 100644 --- a/src/factory/test/UpgradeableModuleProxyFactory.t.sol +++ b/src/factory/test/UpgradeableModuleProxyFactory.t.sol @@ -2,55 +2,160 @@ pragma solidity 0.8.16; import {FirmTest} from "../../common/test/lib/FirmTest.sol"; -import {UpgradeableModuleProxyFactoryMock} from "./mocks/UpgradeableModuleProxyFactoryMock.sol"; +import {IAvatar, SafeAware} from "../../bases/SafeAware.sol"; -contract Target { - error SomeError(); +import {UpgradeableModuleProxyFactory, LATEST_VERSION, Ownable} from "../UpgradeableModuleProxyFactory.sol"; +import {TargetBase, TargetV1, TargetV2, IModuleMetadata} from "./lib/TestTargets.sol"; - function upgrade(address _newTarget) external { - bytes32 slot = bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1); - assembly { - sstore(slot, _newTarget) - } +contract UpgradeableModuleProxyRegistryTest is FirmTest { + UpgradeableModuleProxyFactory factory; + TargetV1 implV1; + + address OWNER = account("Owner"); + address SOMEONE = account("Someone"); + + function setUp() public { + vm.prank(OWNER); + factory = new UpgradeableModuleProxyFactory(); + + implV1 = new TargetV1(); } - function foo() public pure virtual returns (uint256) { - return 42; + function testInitialState() public { + assertEq(factory.owner(), OWNER); + assertEq(factory.latestModuleVersion("org.firm.test-target"), 0); + vm.expectRevert(abi.encodeWithSelector(UpgradeableModuleProxyFactory.UnexistentModuleVersion.selector)); + factory.getImplementation("org.firm.test-target", 1); + vm.expectRevert(abi.encodeWithSelector(UpgradeableModuleProxyFactory.UnexistentModuleVersion.selector)); + factory.getImplementation("org.firm.test-target", LATEST_VERSION); } - function bar() public pure returns (uint256) { - revert SomeError(); + function testOwnerCanRegister() public { + vm.prank(OWNER); + factory.register(implV1); + + assertEq(address(factory.getImplementation("org.firm.test-target", 1)), address(implV1)); + assertEq(address(factory.getImplementation("org.firm.test-target", LATEST_VERSION)), address(implV1)); + assertEq(factory.latestModuleVersion("org.firm.test-target"), implV1.moduleVersion()); + } + + function testCanRegisterMultipleVersions() public { + testOwnerCanRegister(); + + TargetV2 implV2 = new TargetV2(); + vm.prank(OWNER); + factory.register(implV2); + + assertEq(address(factory.getImplementation("org.firm.test-target", 2)), address(implV2)); + assertEq(address(factory.getImplementation("org.firm.test-target", LATEST_VERSION)), address(implV2)); + + assertEq(factory.latestModuleVersion("org.firm.test-target"), implV2.moduleVersion()); + assertEq(address(factory.getImplementation("org.firm.test-target", 1)), address(implV1)); + assertEq(address(factory.getImplementation("org.firm.test-target", 2)), address(implV2)); + } + + function testNotOwnerCannotRegister() public { + vm.prank(SOMEONE); + vm.expectRevert("Ownable: caller is not the owner"); + factory.register(implV1); } -} -contract UpgradedTarget is Target { - function foo() public pure override returns (uint256) { - return 43; + function testCantReregisterSameVersion() public { + vm.prank(OWNER); + factory.register(implV1); + vm.prank(OWNER); + vm.expectRevert(abi.encodeWithSelector(UpgradeableModuleProxyFactory.ModuleVersionAlreadyRegistered.selector)); + factory.register(implV1); + } + + function testOwnerCanChangeOwner() public { + vm.prank(OWNER); + factory.transferOwnership(SOMEONE); + + assertEq(factory.owner(), SOMEONE); + } + + function testNotOwnerCantChangeOwner() public { + vm.prank(SOMEONE); + vm.expectRevert("Ownable: caller is not the owner"); + factory.transferOwnership(OWNER); } } -contract UpgradeableModuleProxyFactoryTest is FirmTest { - Target target = new Target(); - UpgradeableModuleProxyFactoryMock factory = new UpgradeableModuleProxyFactoryMock(); +contract UpgradeableModuleProxyDeployTest is FirmTest { + UpgradeableModuleProxyFactory factory; + address SAFE = account("Safe"); + address SOMEONE = account("Someone"); - Target proxy; + TargetBase proxy; function setUp() public { - proxy = Target(factory.createUpgradeableProxy(address(target))); + factory = new UpgradeableModuleProxyFactory(); + factory.register(new TargetV1()); + factory.register(new TargetV2()); + proxy = TargetBase(factory.deployUpgradeableModule("org.firm.test-target", 1, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), 0)); + } + + function testReturnDataOnUpgradedVersion() public { + proxy.setNumber(42); + assertEq(proxy.getNumber(), 42); + vm.startPrank(SAFE); + proxy.upgrade(factory.getImplementation("org.firm.test-target", 2)); + vm.stopPrank(); + assertEq(proxy.getNumber(), 42 * 2); + } + + function testFirstStorageSlotIsZero() public { + assertEq(vm.load(address(proxy), 0), bytes32(proxy.getNumber())); + proxy.setNumber(42); + assertEq(vm.load(address(proxy), 0), bytes32(proxy.getNumber())); + } + + function testRevertsOnUpgradedVersion() public { + vm.startPrank(SAFE); + proxy.upgrade(factory.getImplementation("org.firm.test-target", 2)); + vm.stopPrank(); + vm.expectRevert(abi.encodeWithSelector(TargetBase.SomeError.selector)); + proxy.setNumber(43); + } + + event ModuleProxyCreation(address indexed proxy, IModuleMetadata indexed implementation); + event Initialized(IAvatar indexed safe, IModuleMetadata indexed implementation); + event Upgraded(IModuleMetadata indexed implementation, string moduleId, uint256 version); + function testEventsAreEmittedInOrder() public { + IModuleMetadata implV1 = factory.getImplementation("org.firm.test-target", 1); + IModuleMetadata implV2 = factory.getImplementation("org.firm.test-target", 2); + + vm.expectEmit(false, true, false, false, address(factory)); + emit ModuleProxyCreation(address(0), implV1); + vm.expectEmit(true, true, false, false); + emit Initialized(IAvatar(SAFE), implV1); + proxy = TargetBase(factory.deployUpgradeableModule("org.firm.test-target", 1, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), 1)); + + vm.prank(SAFE); + vm.expectEmit(true, false, false, true, address(proxy)); + emit Upgraded(implV2, "org.firm.test-target", 2); + proxy.upgrade(implV2); } - function testReturnData() public { - assertEq(proxy.foo(), 42); + function testRevertsIfUpgraderIsNotSafe() public { + IModuleMetadata newImpl = factory.getImplementation("org.firm.test-target", 2); + vm.prank(SOMEONE); + vm.expectRevert(abi.encodeWithSelector(SafeAware.UnauthorizedNotSafe.selector)); + proxy.upgrade(newImpl); } - function testRevert() public { - vm.expectRevert(abi.encodeWithSelector(Target.SomeError.selector)); - proxy.bar(); + function testRevertsIfInitializerReverts() public { + vm.expectRevert(abi.encodeWithSelector(UpgradeableModuleProxyFactory.FailedInitialization.selector)); + factory.deployUpgradeableModule("org.firm.test-target", 2, abi.encodeCall(TargetBase.setNumber, (1)), 1); } - function testUpgrade() public { - assertEq(proxy.foo(), 42); - proxy.upgrade(address(new UpgradedTarget())); - assertEq(proxy.foo(), 43); + function testRevertsIfRepeatingNonce() public { + uint256 nonce = 1; + factory.deployUpgradeableModule("org.firm.test-target", 1, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), nonce); + factory.deployUpgradeableModule("org.firm.test-target", 2, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), nonce); + + vm.expectRevert(abi.encodeWithSelector(UpgradeableModuleProxyFactory.ProxyAlreadyDeployedForNonce.selector)); + factory.deployUpgradeableModule("org.firm.test-target", 1, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), nonce); } } diff --git a/src/factory/test/lib/TestTargets.sol b/src/factory/test/lib/TestTargets.sol new file mode 100644 index 0000000..9c76c5f --- /dev/null +++ b/src/factory/test/lib/TestTargets.sol @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.16; + +import {FirmBase, IModuleMetadata} from "../../../bases/FirmBase.sol"; +import {IAvatar} from "../../../bases/SafeAware.sol"; + +abstract contract TargetBase is FirmBase { + string public constant moduleId = "org.firm.test-target"; + + uint256 public someNumber; + + error SomeError(); + + function init(IAvatar safe) public { + __init_firmBase(safe, address(0)); + } + + function setNumber(uint256 number) public virtual; + function getNumber() public view virtual returns (uint256); +} + +contract TargetV1 is TargetBase { + uint256 public constant moduleVersion = 1; + + function setNumber(uint256 number) public override { + someNumber = number; + } + + function getNumber() public override view returns (uint256) { + return someNumber; + } +} + +contract TargetV2 is TargetBase { + uint256 public constant moduleVersion = 2; + + function setNumber(uint256) public pure override { + revert SomeError(); + } + + function getNumber() public override view returns (uint256) { + return someNumber * 2; + } +} diff --git a/src/factory/test/mocks/UpgradeableModuleProxyFactoryMock.sol b/src/factory/test/mocks/UpgradeableModuleProxyFactoryMock.sol deleted file mode 100644 index bcb1ec5..0000000 --- a/src/factory/test/mocks/UpgradeableModuleProxyFactoryMock.sol +++ /dev/null @@ -1,10 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity 0.8.16; - -import "../../UpgradeableModuleProxyFactory.sol"; - -contract UpgradeableModuleProxyFactoryMock is UpgradeableModuleProxyFactory { - function createUpgradeableProxy(address _target) public returns (address addr) { - return createUpgradeableProxy(EIP1967Upgradeable(_target), bytes32(0)); - } -} From d2dadbdac208e55095c131744987ce0211dcd52c Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 20 Oct 2022 17:02:10 +0200 Subject: [PATCH 10/13] General: make event naming consistent (#82) * fix(events): make event naming consistent * chore: revert zodiac-related changes --- src/factory/FirmFactory.sol | 8 ++++---- src/factory/UpgradeableModuleProxyFactory.sol | 4 ++-- src/factory/test/FirmFactoryIntegrationTest.t.sol | 4 ++-- src/factory/test/UpgradeableModuleProxyFactory.t.sol | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/factory/FirmFactory.sol b/src/factory/FirmFactory.sol index e35f489..a102d3b 100644 --- a/src/factory/FirmFactory.sol +++ b/src/factory/FirmFactory.sol @@ -26,8 +26,8 @@ contract FirmFactory { error EnableModuleFailed(); - event NewFirm(address indexed creator, GnosisSafe indexed safe, Roles roles, Budget budget); - event DeployedBackdoors(GnosisSafe indexed safe, address[] backdoors); + event NewFirmCreated(address indexed creator, GnosisSafe indexed safe, Roles roles, Budget budget); + event BackdoorsDeployed(GnosisSafe indexed safe, address[] backdoors); constructor( GnosisSafeProxyFactory _safeFactory, @@ -71,12 +71,12 @@ contract FirmFactory { Budget budget = Budget(modules[0]); Roles roles = Roles(address(budget.roles())); - emit NewFirm(msg.sender, safe, roles, budget); + emit NewFirmCreated(msg.sender, safe, roles, budget); if (withBackdoors) { (address[] memory backdoors,) = safe.getModulesPaginated(address(budget), 2); - emit DeployedBackdoors(safe, backdoors); + emit BackdoorsDeployed(safe, backdoors);(safe, backdoors); } } diff --git a/src/factory/UpgradeableModuleProxyFactory.sol b/src/factory/UpgradeableModuleProxyFactory.sol index 0e6bde7..10cf6aa 100644 --- a/src/factory/UpgradeableModuleProxyFactory.sol +++ b/src/factory/UpgradeableModuleProxyFactory.sol @@ -13,7 +13,7 @@ contract UpgradeableModuleProxyFactory is Ownable { error UnexistentModuleVersion(); event ModuleRegistered(IModuleMetadata indexed implementation, string moduleId, uint256 version); - event ModuleProxyCreation(address indexed proxy, IModuleMetadata indexed implementation); + event ModuleProxyCreated(address indexed proxy, IModuleMetadata indexed implementation); mapping(string => mapping(uint256 => IModuleMetadata)) internal modules; mapping(string => uint256) public latestModuleVersion; @@ -79,6 +79,6 @@ contract UpgradeableModuleProxyFactory is Ownable { revert ProxyAlreadyDeployedForNonce(); } - emit ModuleProxyCreation(proxy, implementation); + emit ModuleProxyCreated(proxy, implementation); } } diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index c2bba83..3f20e41 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -42,12 +42,12 @@ contract FirmFactoryIntegrationTest is FirmTest { createFirm(address(this)); } - event NewFirm(address indexed creator, GnosisSafe indexed safe, Roles roles, Budget budget); + event NewFirmCreated(address indexed creator, GnosisSafe indexed safe, Roles roles, Budget budget); function testInitialState() public { // we don't match the deployed contract addresses for simplicity (could precalculate them but unnecessary) vm.expectEmit(true, false, false, false); - emit NewFirm(address(this), GnosisSafe(payable(0)), Roles(address(0)), Budget(address(0))); + emit NewFirmCreated(address(this), GnosisSafe(payable(0)), Roles(address(0)), Budget(address(0))); (GnosisSafe safe, Budget budget, Roles roles) = createFirm(address(this)); diff --git a/src/factory/test/UpgradeableModuleProxyFactory.t.sol b/src/factory/test/UpgradeableModuleProxyFactory.t.sol index 6d37132..727ea4c 100644 --- a/src/factory/test/UpgradeableModuleProxyFactory.t.sol +++ b/src/factory/test/UpgradeableModuleProxyFactory.t.sol @@ -119,7 +119,7 @@ contract UpgradeableModuleProxyDeployTest is FirmTest { proxy.setNumber(43); } - event ModuleProxyCreation(address indexed proxy, IModuleMetadata indexed implementation); + event ModuleProxyCreated(address indexed proxy, IModuleMetadata indexed implementation); event Initialized(IAvatar indexed safe, IModuleMetadata indexed implementation); event Upgraded(IModuleMetadata indexed implementation, string moduleId, uint256 version); function testEventsAreEmittedInOrder() public { @@ -127,7 +127,7 @@ contract UpgradeableModuleProxyDeployTest is FirmTest { IModuleMetadata implV2 = factory.getImplementation("org.firm.test-target", 2); vm.expectEmit(false, true, false, false, address(factory)); - emit ModuleProxyCreation(address(0), implV1); + emit ModuleProxyCreated(address(0), implV1); vm.expectEmit(true, true, false, false); emit Initialized(IAvatar(SAFE), implV1); proxy = TargetBase(factory.deployUpgradeableModule("org.firm.test-target", 1, abi.encodeCall(TargetBase.init, (IAvatar(SAFE))), 1)); From 8516175bcab1e04771537478573e07e9bdc6acd3 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 21 Oct 2022 12:17:43 +0200 Subject: [PATCH 11/13] Remove RecurringPayments from branch scope --- src/budget/modules/RecurringPayments.sol | 131 ----------------------- 1 file changed, 131 deletions(-) delete mode 100644 src/budget/modules/RecurringPayments.sol diff --git a/src/budget/modules/RecurringPayments.sol b/src/budget/modules/RecurringPayments.sol deleted file mode 100644 index 37c9a6e..0000000 --- a/src/budget/modules/RecurringPayments.sol +++ /dev/null @@ -1,131 +0,0 @@ -// 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()); - } -} From 55af06c22aa2d0a0de00bfc685f72243e778bd08 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 21 Oct 2022 12:21:19 +0200 Subject: [PATCH 12/13] Remove Recurring Payments integration test --- .../test/FirmFactoryIntegrationTest.t.sol | 64 ------------------- 1 file changed, 64 deletions(-) diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index 3f20e41..97bc494 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -14,7 +14,6 @@ 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"; @@ -27,8 +26,6 @@ contract FirmFactoryIntegrationTest is FirmTest { FirmRelayer relayer; ERC20Token token; - RecurringPayments immutable recurringPaymentsImpl = new RecurringPayments(); - function setUp() public { token = new ERC20Token(); @@ -122,67 +119,6 @@ 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 222638785c6c45c063bfba1d20d3709eece87679 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 21 Oct 2022 12:49:13 +0200 Subject: [PATCH 13/13] BudgetModule: unstructured storage --- src/budget/modules/BudgetModule.sol | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/budget/modules/BudgetModule.sol b/src/budget/modules/BudgetModule.sol index d056c97..a287431 100644 --- a/src/budget/modules/BudgetModule.sol +++ b/src/budget/modules/BudgetModule.sol @@ -7,7 +7,8 @@ import {FirmBase, IAvatar} from "../../bases/FirmBase.sol"; address constant IMPL_INIT_ADDRESS = address(1); abstract contract BudgetModule is FirmBase { - Budget public budget; // TODO: Unstructured storage + // BUDGET_SLOT = keccak256("firm.budgetmodule.budget") - 1 + bytes32 internal constant BUDGET_SLOT = 0xc7637e5414363c2355f9e835e00d15501df0666fb3c6c5fe259b9a40aeedbc49; constructor() { // Initialize with impossible values in constructor so impl base cannot be used @@ -17,14 +18,22 @@ abstract contract BudgetModule is FirmBase { function initialize(Budget budget_, address trustedForwarder_) public { IAvatar safe = address(budget_) != IMPL_INIT_ADDRESS ? budget_.safe() : IAvatar(IMPL_INIT_ADDRESS); __init_firmBase(safe, trustedForwarder_); - budget = budget_; + assembly { + sstore(BUDGET_SLOT, budget_) + } + } + + function budget() public view returns (Budget _budget) { + assembly { + _budget := sload(BUDGET_SLOT) + } } error UnauthorizedNotAllowanceAdmin(uint256 allowanceId, address actor); modifier onlyAllowanceAdmin(uint256 allowanceId) { address actor = _msgSender(); - if (!budget.isAdminOnAllowance(allowanceId, actor)) { + if (!budget().isAdminOnAllowance(allowanceId, actor)) { revert UnauthorizedNotAllowanceAdmin(allowanceId, actor); }