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 9823b5f..6452021 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); @@ -95,6 +96,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 +132,13 @@ 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(); + } + // 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 @@ -200,6 +209,11 @@ contract Budget is FirmBase, ZodiacModule, RolesAuth { */ function setAllowanceAmount(uint256 allowanceId, uint256 amount) external { Allowance storage allowance = _getAllowanceAndValidateAdmin(allowanceId); + + if (amount == INHERITED_AMOUNT && allowance.parentId == NO_PARENT_ID) { + revert ZeroAmountForTopLevelAllowance(); + } + allowance.amount = amount; emit AllowanceAmountChanged(allowanceId, amount); } @@ -236,7 +250,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 { + 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 +267,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 +288,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 +309,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)) { @@ -320,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)); } @@ -375,6 +392,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 +436,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/BudgetModule.sol b/src/budget/modules/BudgetModule.sol new file mode 100644 index 0000000..a287431 --- /dev/null +++ b/src/budget/modules/BudgetModule.sol @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: UNLICENSED +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_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 + initialize(Budget(IMPL_INIT_ADDRESS), IMPL_INIT_ADDRESS); + } + + function initialize(Budget budget_, address trustedForwarder_) public { + IAvatar safe = address(budget_) != IMPL_INIT_ADDRESS ? budget_.safe() : IAvatar(IMPL_INIT_ADDRESS); + __init_firmBase(safe, trustedForwarder_); + 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)) { + revert UnauthorizedNotAllowanceAdmin(allowanceId, actor); + } + + _; + } +} 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/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 fba6120..10cf6aa 100644 --- a/src/factory/UpgradeableModuleProxyFactory.sol +++ b/src/factory/UpgradeableModuleProxyFactory.sol @@ -1,43 +1,84 @@ // 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 ModuleProxyCreated(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); + emit ModuleProxyCreated(proxy, implementation); } } diff --git a/src/factory/test/FirmFactoryIntegrationTest.t.sol b/src/factory/test/FirmFactoryIntegrationTest.t.sol index cdabf21..97bc494 100644 --- a/src/factory/test/FirmFactoryIntegrationTest.t.sol +++ b/src/factory/test/FirmFactoryIntegrationTest.t.sol @@ -15,6 +15,7 @@ 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 {SafeEnums} from "../../bases/IZodiacModule.sol"; +import {BokkyPooBahsDateTimeLibrary as DateTimeLib} from "datetime/BokkyPooBahsDateTimeLibrary.sol"; import {LocalDeploy} from "../../../scripts/LocalDeploy.sol"; @@ -38,12 +39,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)); @@ -121,9 +122,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..727ea4c 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 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 { + 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 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)); + + 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)); - } -}