diff --git a/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol b/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol index 72d712a..950ae7e 100644 --- a/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol +++ b/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol @@ -28,15 +28,28 @@ contract MemberAccessExecuteCondition is PermissionCondition { ) external view returns (bool) { (_where, _who, _permissionId); - if ( - getSelector(_data) != MainVotingPlugin.addMember.selector && - getSelector(_data) != MainVotingPlugin.removeMember.selector - ) { - return false; - } else if (_where != targetContract) { + // Is it execute()? + if (getSelector(_data) != IDAO.execute.selector) { return false; } + (, IDAO.Action[] memory _actions, ) = abi.decode( + _data[4:], + (bytes32, IDAO.Action[], uint256) + ); + + // Check actions + if (_actions.length != 1) return false; + else if (_actions[0].to != targetContract) return false; + + // Decode the call being requested (both have the same parameters) + (bytes4 _requestedSelector, ) = decodeAddRemoveMemberCalldata(_actions[0].data); + + if ( + _requestedSelector != MainVotingPlugin.addMember.selector && + _requestedSelector != MainVotingPlugin.removeMember.selector + ) return false; + return true; } @@ -47,4 +60,15 @@ contract MemberAccessExecuteCondition is PermissionCondition { selector := mload(add(_data, 0x20)) // 32 } } + + function decodeAddRemoveMemberCalldata( + bytes memory _data + ) public pure returns (bytes4 sig, address account) { + // Slicing is only supported for bytes calldata, not bytes memory + // Bytes memory requires an assembly block + assembly { + sig := mload(add(_data, 0x20)) // 32 + account := mload(add(_data, 0x24)) // 32 + 4 + } + } } diff --git a/packages/contracts/test/integration-testing/member-access-condition.ts b/packages/contracts/test/integration-testing/member-access-condition.ts index 6e3f63a..fdecbb1 100644 --- a/packages/contracts/test/integration-testing/member-access-condition.ts +++ b/packages/contracts/test/integration-testing/member-access-condition.ts @@ -31,7 +31,6 @@ import { PluginSetupProcessor__factory, PluginRepoFactory__factory, PluginRepoRegistry__factory, - IDAO, DAO__factory, } from '@aragon/osx-ethers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; @@ -46,8 +45,9 @@ const pluginSettings: MajorityVotingBase.VotingSettingsStruct = { supportThreshold: 1, votingMode: 0, }; -const minMemberAccessProposalDuration = 60 * 60 * 24; +const memberAccessProposalDuration = 60 * 60 * 24; const daoInterface = DAO__factory.createInterface(); +const mainVotingInterface = MainVotingPlugin__factory.createInterface(); describe('Member Access Condition E2E', () => { let deployer: SignerWithAddress; @@ -149,7 +149,7 @@ describe('Member Access Condition E2E', () => { const data = await pluginSetup.encodeInstallationParams( pluginSettings, [deployer.address], - minMemberAccessProposalDuration, + memberAccessProposalDuration, pluginUpgrader.address ); const installation = await installPlugin(psp, dao, pluginSetupRef, data); @@ -165,61 +165,33 @@ describe('Member Access Condition E2E', () => { }); it('Executing a proposal to add membership works', async () => { - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - alice.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(false); + expect(await mainVotingPlugin.isMember(alice.address)).to.eq(false); + + await memberAccessPlugin.proposeNewMember('0x', alice.address); await expect(memberAccessPlugin.proposeNewMember('0x', alice.address)).to .not.be.reverted; - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - alice.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(true); - - // Valid grant - const actions: IDAO.ActionStruct[] = [ - { - to: dao.address, - value: 0, - data: daoInterface.encodeFunctionData('grant', [ - mainVotingPlugin.address, - bob.address, - MEMBER_PERMISSION_ID, - ]), - }, - ]; + expect(await mainVotingPlugin.isMember(alice.address)).to.eq(true); - // Via direct create proposal - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - bob.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(false); - - await expect(memberAccessPlugin.createArbitraryProposal('0x', actions)).to - .not.be.reverted; + // // Valid addition + // const actions: IDAO.ActionStruct[] = [ + // { + // to: mainVotingPlugin.address, + // value: 0, + // data: mainVotingInterface.encodeFunctionData('addMember', [ + // bob.address, + // ]), + // }, + // ]; - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - bob.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(true); + // // Via direct create proposal + // expect(await mainVotingPlugin.isMember(bob.address)).to.eq(false); + + // await expect(memberAccessPlugin.createArbitraryProposal('0x', actions)).to + // .not.be.reverted; + + // expect(await mainVotingPlugin.isMember(bob.address)).to.eq(true); }); it('Executing a proposal to remove membership works', async () => { @@ -227,32 +199,19 @@ describe('Member Access Condition E2E', () => { .not.be.reverted; await expect(memberAccessPlugin.proposeRemoveMember('0x', alice.address)).to .not.be.reverted; - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - alice.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(false); + expect(await mainVotingPlugin.isMember(alice.address)).to.eq(false); // Valid revoke const grantAction = { - to: dao.address, + to: mainVotingPlugin.address, value: 0, - data: daoInterface.encodeFunctionData('grant', [ - mainVotingPlugin.address, - bob.address, - MEMBER_PERMISSION_ID, - ]), + data: mainVotingInterface.encodeFunctionData('addMember', [bob.address]), }; const revokeAction = { - to: dao.address, + to: mainVotingPlugin.address, value: 0, - data: daoInterface.encodeFunctionData('revoke', [ - mainVotingPlugin.address, + data: mainVotingInterface.encodeFunctionData('removeMember', [ bob.address, - MEMBER_PERMISSION_ID, ]), }; @@ -260,38 +219,29 @@ describe('Member Access Condition E2E', () => { await expect( memberAccessPlugin.createArbitraryProposal('0x', [grantAction]) ).to.not.be.reverted; + expect(await mainVotingPlugin.isMember(bob.address)).to.eq(true); + await expect( memberAccessPlugin.createArbitraryProposal('0x', [revokeAction]) ).to.not.be.reverted; - expect( - await dao.isGranted( - mainVotingPlugin.address, // where - bob.address, // who - MEMBER_PERMISSION_ID, // permission - '0x' - ) - ).to.eq(false); + expect(await mainVotingPlugin.isMember(bob.address)).to.eq(false); }); it('Executing a proposal to do something else reverts', async () => { const validActions = [ { - to: dao.address, + to: mainVotingPlugin.address, value: 0, - data: daoInterface.encodeFunctionData('grant', [ - mainVotingPlugin.address, + data: mainVotingInterface.encodeFunctionData('addMember', [ bob.address, - MEMBER_PERMISSION_ID, ]), }, { - to: dao.address, + to: mainVotingPlugin.address, value: 0, - data: daoInterface.encodeFunctionData('revoke', [ - mainVotingPlugin.address, + data: mainVotingInterface.encodeFunctionData('removeMember', [ bob.address, - MEMBER_PERMISSION_ID, ]), }, ]; diff --git a/packages/contracts/test/unit-testing/member-access-execute-condition.ts b/packages/contracts/test/unit-testing/member-access-execute-condition.ts index 1879525..9758489 100644 --- a/packages/contracts/test/unit-testing/member-access-execute-condition.ts +++ b/packages/contracts/test/unit-testing/member-access-execute-condition.ts @@ -2,6 +2,7 @@ import { DAO, DAO__factory, IDAO, + MainVotingPlugin__factory, MemberAccessExecuteCondition, MemberAccessExecuteCondition__factory, } from '../../typechain'; @@ -29,6 +30,7 @@ const ONE_BYTES32 = const PLUGIN_ADDR_1 = ADDRESS_ONE; const PLUGIN_ADDR_2 = ADDRESS_TWO; const daoInterface = DAO__factory.createInterface(); +const mainVotingPluginInterface = MainVotingPlugin__factory.createInterface(); describe('Member Access Condition', function () { const pspAddress = getPluginSetupProcessorAddress(network.name, true); @@ -49,18 +51,17 @@ describe('Member Access Condition', function () { memberAccessExecuteCondition = await factory.deploy(SOME_CONTRACT_ADDRESS); }); - describe('Executing grant/revoke MEMBER_PERMISSION_ID on a certain contract', () => { - it('Should only allow executing grant and revoke', async () => { + describe('Executing addMember and removeMember on a certain contract', () => { + it('Should only allow executing addMember and removeMember', async () => { const actions: IDAO.ActionStruct[] = [ - {to: dao.address, value: 0, data: '0x'}, + {to: SOME_CONTRACT_ADDRESS, value: 0, data: '0x'}, ]; - // Valid grant - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); + // Valid add + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'addMember', + [carol.address] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -70,12 +71,11 @@ describe('Member Access Condition', function () { ) ).to.eq(true); - // Valid revoke - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); + // Valid remove + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'removeMember', + [carol.address] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -126,140 +126,16 @@ describe('Member Access Condition', function () { ).to.eq(false); }); - it('Should only allow MEMBER_PERMISSION_ID', async () => { - const actions: IDAO.ActionStruct[] = [ - {to: dao.address, value: 0, data: '0x'}, - ]; - - // Valid grant - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(true); - - // Valid revoke - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(true); - - // Invalid - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - EDITOR_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - EDITOR_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - - // Invalid - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - ROOT_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - ROOT_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - - // Invalid - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - DEPLOYER_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - DEPLOYER_PERMISSION_ID, - ]); - expect( - await memberAccessExecuteCondition.isGranted( - ADDRESS_ONE, // where (used) - ADDRESS_TWO, // who (used) - EXECUTE_PERMISSION_ID, // permission (used) - daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) - ) - ).to.eq(false); - }); - it('Should only allow to target the intended plugin contract', async () => { const actions: IDAO.ActionStruct[] = [ - {to: dao.address, value: 0, data: '0x'}, + {to: SOME_CONTRACT_ADDRESS, value: 0, data: '0x'}, ]; // Valid grant - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'addMember', + [carol.address] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -270,11 +146,10 @@ describe('Member Access Condition', function () { ).to.eq(true); // Valid revoke - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - carol.address, - MEMBER_PERMISSION_ID, - ]); + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'removeMember', + [carol.address] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -284,6 +159,34 @@ describe('Member Access Condition', function () { ) ).to.eq(true); + // // Invalid + // actions[0].data = mainVotingPluginInterface.encodeFunctionData( + // 'removeEditor', + // [carol.address] + // ); + // expect( + // await memberAccessExecuteCondition.isGranted( + // ADDRESS_ONE, // where (used) + // ADDRESS_TWO, // who (used) + // EXECUTE_PERMISSION_ID, // permission (used) + // daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) + // ) + // ).to.eq(true); + + // // Invalid + // actions[0].data = mainVotingPluginInterface.encodeFunctionData( + // 'addEditor', + // [carol.address] + // ); + // expect( + // await memberAccessExecuteCondition.isGranted( + // ADDRESS_ONE, // where (used) + // ADDRESS_TWO, // who (used) + // EXECUTE_PERMISSION_ID, // permission (used) + // daoInterface.encodeFunctionData('execute', [ONE_BYTES32, actions, 0]) + // ) + // ).to.eq(true); + // Invalid actions[0].data = daoInterface.encodeFunctionData('grant', [ ADDRESS_TWO, @@ -343,9 +246,9 @@ describe('Member Access Condition', function () { ).to.eq(false); }); - it("Should allow granting to whatever 'who' address", async () => { + it('Should allow adding/removing any address', async () => { const actions: IDAO.ActionStruct[] = [ - {to: dao.address, value: 0, data: '0x'}, + {to: SOME_CONTRACT_ADDRESS, value: 0, data: '0x'}, ]; for (const grantedToAddress of [ SOME_CONTRACT_ADDRESS, @@ -353,12 +256,11 @@ describe('Member Access Condition', function () { dao.address, ADDRESS_ONE, ]) { - // Valid grant - actions[0].data = daoInterface.encodeFunctionData('grant', [ - SOME_CONTRACT_ADDRESS, - grantedToAddress, - MEMBER_PERMISSION_ID, - ]); + // Valid add + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'addMember', + [grantedToAddress] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -372,12 +274,11 @@ describe('Member Access Condition', function () { ) ).to.eq(true); - // Valid revoke - actions[0].data = daoInterface.encodeFunctionData('revoke', [ - SOME_CONTRACT_ADDRESS, - grantedToAddress, - MEMBER_PERMISSION_ID, - ]); + // Valid remove + actions[0].data = mainVotingPluginInterface.encodeFunctionData( + 'removeMember', + [grantedToAddress] + ); expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) @@ -394,19 +295,16 @@ describe('Member Access Condition', function () { }); }); - describe('Direct grant and revoke are not allowed', () => { - it('Should reject granting and revoking directly', async () => { + describe('Direct add and remove are not allowed', () => { + it('Should reject adding and removing directly, rather than executing', async () => { // Valid expect( await memberAccessExecuteCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) - DAO__factory.createInterface().encodeFunctionData('grant', [ - // call - SOME_CONTRACT_ADDRESS, + mainVotingPluginInterface.encodeFunctionData('addMember', [ carol.address, - MEMBER_PERMISSION_ID, ]) ) ).to.eq(false); @@ -416,11 +314,8 @@ describe('Member Access Condition', function () { ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) - DAO__factory.createInterface().encodeFunctionData('revoke', [ - // call - SOME_CONTRACT_ADDRESS, + mainVotingPluginInterface.encodeFunctionData('removeMember', [ carol.address, - MEMBER_PERMISSION_ID, ]) ) ).to.eq(false); @@ -433,19 +328,15 @@ describe('Member Access Condition', function () { { to: dao.address, value: 0, - data: daoInterface.encodeFunctionData('grant', [ - PLUGIN_ADDR_1, + data: mainVotingPluginInterface.encodeFunctionData('addMember', [ pspAddress, - MEMBER_PERMISSION_ID, ]), }, { to: dao.address, value: 0, - data: daoInterface.encodeFunctionData('revoke', [ - PLUGIN_ADDR_1, + data: mainVotingPluginInterface.encodeFunctionData('removeMember', [ pspAddress, - MEMBER_PERMISSION_ID, ]), }, ]; @@ -458,5 +349,30 @@ describe('Member Access Condition', function () { await memberAccessExecuteCondition.getSelector(actions[1].data) ).to.eq((actions[1].data as string).slice(0, 10)); }); + + it('Should decode decodeGrantRevokeCalldata properly', async () => { + const calldataList = [ + mainVotingPluginInterface.encodeFunctionData('addMember', [pspAddress]), + mainVotingPluginInterface.encodeFunctionData('removeMember', [ + bob.address, + ]), + ]; + + // 1 + let [selector, who] = + await memberAccessExecuteCondition.decodeAddRemoveMemberCalldata( + calldataList[0] + ); + expect(selector).to.eq(calldataList[0].slice(0, 10)); + expect(who).to.eq(pspAddress); + + // 2 + [selector, who] = + await memberAccessExecuteCondition.decodeAddRemoveMemberCalldata( + calldataList[1] + ); + expect(selector).to.eq(calldataList[1].slice(0, 10)); + expect(who).to.eq(bob.address); + }); }); });