diff --git a/packages/contracts/src/GovernancePluginsSetup.sol b/packages/contracts/src/GovernancePluginsSetup.sol index 025ca59..d6fc786 100644 --- a/packages/contracts/src/GovernancePluginsSetup.sol +++ b/packages/contracts/src/GovernancePluginsSetup.sol @@ -104,7 +104,7 @@ contract GovernancePluginsSetup is PluginSetup { // The member access plugin needs to execute on the DAO permissions[3] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Grant, + operation: PermissionLib.Operation.GrantWithCondition, where: _dao, who: _memberAccessPlugin, condition: _memberAccessExecuteCondition, @@ -135,7 +135,7 @@ contract GovernancePluginsSetup is PluginSetup { _targetPluginAddresses ); permissions[5] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Grant, + operation: PermissionLib.Operation.GrantWithCondition, where: _dao, who: _pluginUpgrader, condition: address(_onlyPluginUpgraderCondition), diff --git a/packages/contracts/src/OnlyPluginUpgraderCondition.sol b/packages/contracts/src/OnlyPluginUpgraderCondition.sol index 9446c20..f2a5bcc 100644 --- a/packages/contracts/src/OnlyPluginUpgraderCondition.sol +++ b/packages/contracts/src/OnlyPluginUpgraderCondition.sol @@ -139,9 +139,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { ) = decodeGrantRevokeCalldata(_grantData); if (_grantSelector != PermissionManager.grant.selector) return false; + else if (!allowedPluginAddresses[_grantWhere]) return false; else if (_grantWho != psp) return false; else if (_grantPermission != UPGRADE_PLUGIN_PERMISSION_ID) return false; - else if (!allowedPluginAddresses[_grantWhere]) return false; // Revoke checks ( @@ -152,9 +152,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { ) = decodeGrantRevokeCalldata(_revokeData); if (_revokeSelector != PermissionManager.revoke.selector) return false; + else if (!allowedPluginAddresses[_revokeWhere]) return false; else if (_revokeWho != psp) return false; else if (_revokePermission != UPGRADE_PLUGIN_PERMISSION_ID) return false; - else if (!allowedPluginAddresses[_revokeWhere]) return false; // Combined checks if (_grantWhere != _revokeWhere) return false; diff --git a/packages/contracts/test/integration-testing/governance-plugins-setup.ts b/packages/contracts/test/integration-testing/governance-plugins-setup.ts index 88b9bde..1b71928 100644 --- a/packages/contracts/test/integration-testing/governance-plugins-setup.ts +++ b/packages/contracts/test/integration-testing/governance-plugins-setup.ts @@ -10,7 +10,10 @@ import { PluginRepo, } from '../../typechain'; import {PluginSetupRefStruct} from '../../typechain/@aragon/osx/framework/dao/DAOFactory'; -import {UpdatePreparedEvent} from '../../typechain/@aragon/osx/framework/plugin/setup/PluginSetupProcessor'; +import { + UpdateAppliedEvent, + UpdatePreparedEvent, +} from '../../typechain/@aragon/osx/framework/plugin/setup/PluginSetupProcessor'; import { findEvent, findEventTopicLog, @@ -186,6 +189,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { // Deploy DAO. dao = await deployTestDao(deployer); + // Permissions (build 1 install) await dao.grant( dao.address, psp.address, @@ -239,7 +243,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { ); }); - it('Allows pluginUpgrader to call applyUpdate', async () => { + it('Allows pluginUpgrader to execute psp.applyUpdate()', async () => { const pluginSetupRef1: PluginSetupRefStruct = { versionTag: { release, @@ -247,13 +251,6 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { }, pluginSetupRepo: pluginRepo.address, }; - const pluginSetupRef2: PluginSetupRefStruct = { - versionTag: { - release, - build: 2, - }, - pluginSetupRepo: pluginRepo.address, - }; // Install build 1 const data1 = await pSetupBuild1.encodeInstallationParams( @@ -285,6 +282,12 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { // Deploy PluginSetup build 2 (new instance, disregarding the lack of changes) const pSetupBuild2 = await gpsFactory.deploy(psp.address); + // Check + expect(await pSetupBuild1.implementation()).to.not.be.eq( + await pSetupBuild2.implementation(), + 'Builds 1-2 implementation should differ' + ); + // Publish build 2 let tx = await pluginRepo.createVersion( 1, @@ -292,6 +295,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { toHex('build'), toHex('release') ); + await tx.wait(); // Upgrade to build 2 tx = await psp.prepareUpdate(dao.address, { @@ -306,7 +310,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { pluginSetupRepo: pluginRepo.address, setupPayload: { currentHelpers: [memberAccessPlugin.address], - data: toHex(''), + data: '0x', plugin: mainVotingPlugin.address, }, }); @@ -318,6 +322,24 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { throw new Error('Failed to get UpdatePrepared event'); } + // Should not allow to execute other than the expected 3 actions + await expect(dao.execute(toHex('01234123412341234123412341234123'), [], 0)) + .to.be.reverted; + await expect( + dao + .connect(pluginUpgrader) + .execute(toHex('01234123412341234123412341234123'), [], 0) + ).to.be.reverted; + await expect( + dao + .connect(pluginUpgrader) + .execute( + toHex('01234123412341234123412341234123'), + [{to: dao.address, value: 0, data: '0x'}], + 0 + ) + ).to.be.reverted; + // Params const applyUpdateParams: PluginSetupProcessor.ApplyUpdateParamsStruct = { plugin: mainVotingPlugin.address, @@ -334,8 +356,8 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => { }; // Execute grant + applyUpdate + revoke - dao.connect(pluginUpgrader).execute( - toHex(Date.now().toString()), + await dao.connect(pluginUpgrader).execute( + toHex('12341234123412341234123412341234'), [ // Grant permission to the PSP {