diff --git a/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol b/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol index b67e96e..69d826d 100644 --- a/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol +++ b/packages/contracts/src/conditions/MemberAccessExecuteCondition.sol @@ -28,7 +28,7 @@ contract MemberAccessExecuteCondition is PermissionCondition { (_where, _who, _permissionId); // Is it execute()? - if (getSelector(_data) != IDAO.execute.selector) { + if (_getSelector(_data) != IDAO.execute.selector) { return false; } @@ -42,7 +42,7 @@ contract MemberAccessExecuteCondition is PermissionCondition { else if (_actions[0].to != targetContract) return false; // Decode the call being requested (both have the same parameters) - (bytes4 _requestedSelector, ) = decodeAddRemoveMemberCalldata(_actions[0].data); + (bytes4 _requestedSelector, ) = _decodeAddRemoveMemberCalldata(_actions[0].data); if ( _requestedSelector != MainVotingPlugin.addMember.selector && @@ -52,7 +52,7 @@ contract MemberAccessExecuteCondition is PermissionCondition { return true; } - function getSelector(bytes memory _data) internal pure returns (bytes4 selector) { + function _getSelector(bytes memory _data) internal pure returns (bytes4 selector) { // Slices are only supported for bytes calldata, not bytes memory // Bytes memory requires an assembly block assembly { @@ -60,7 +60,7 @@ contract MemberAccessExecuteCondition is PermissionCondition { } } - function decodeAddRemoveMemberCalldata( + function _decodeAddRemoveMemberCalldata( bytes memory _data ) internal pure returns (bytes4 sig, address account) { // Slicing is only supported for bytes calldata, not bytes memory diff --git a/packages/contracts/src/conditions/OnlyPluginUpgraderCondition.sol b/packages/contracts/src/conditions/OnlyPluginUpgraderCondition.sol index f2a5bcc..d099376 100644 --- a/packages/contracts/src/conditions/OnlyPluginUpgraderCondition.sol +++ b/packages/contracts/src/conditions/OnlyPluginUpgraderCondition.sol @@ -53,7 +53,7 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { ) external view returns (bool) { (_where, _who, _permissionId); - bytes4 _requestedFuncSig = getSelector(_data); + bytes4 _requestedFuncSig = _getSelector(_data); if (_requestedFuncSig != IDAO.execute.selector) { return false; } @@ -69,16 +69,18 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { // Action 1/3: GRANT/REVOKE UPGRADE_PLUGIN_PERMISSION_ID to the PSP on targetPlugis[] if (_actions[0].to != dao || _actions[2].to != dao) return false; - else if (!isValidGrantRevokeCalldata(_actions[0].data, _actions[2].data)) return false; + else if (!_isValidGrantRevokeCalldata(_actions[0].data, _actions[2].data)) return false; // Action 2: CALL PSP.applyUpdate() onto targetPlugins[] if (_actions[1].to != psp) return false; - else if (!isValidApplyUpdateCalldata(_actions[1].data)) return false; + else if (!_isValidApplyUpdateCalldata(_actions[1].data)) return false; return true; } - function getSelector(bytes memory _data) public pure returns (bytes4 selector) { + // Internal helpers + + function _getSelector(bytes memory _data) internal pure returns (bytes4 selector) { // Slices are only supported for bytes calldata // Bytes memory requires an assembly block assembly { @@ -86,9 +88,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { } } - function decodeGrantRevokeCalldata( + function _decodeGrantRevokeCalldata( bytes memory _data - ) public pure returns (bytes4 selector, address where, address who, bytes32 permissionId) { + ) internal pure returns (bytes4 selector, address where, address who, bytes32 permissionId) { // Slicing is only supported for bytes calldata, not bytes memory // Bytes memory requires an assembly block assembly { @@ -99,9 +101,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { } } - function decodeApplyUpdateCalldata( + function _decodeApplyUpdateCalldata( bytes memory _data - ) public pure returns (bytes4 selector, address daoAddress, address targetPluginAddress) { + ) internal pure returns (bytes4 selector, address daoAddress, address targetPluginAddress) { // Slicing is only supported for bytes calldata, not bytes memory // Bytes memory requires an assembly block assembly { @@ -124,19 +126,17 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { } } - // Internal helpers - - function isValidGrantRevokeCalldata( + function _isValidGrantRevokeCalldata( bytes memory _grantData, bytes memory _revokeData - ) private view returns (bool) { + ) internal view returns (bool) { // Grant checks ( bytes4 _grantSelector, address _grantWhere, address _grantWho, bytes32 _grantPermission - ) = decodeGrantRevokeCalldata(_grantData); + ) = _decodeGrantRevokeCalldata(_grantData); if (_grantSelector != PermissionManager.grant.selector) return false; else if (!allowedPluginAddresses[_grantWhere]) return false; @@ -149,7 +149,7 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { address _revokeWhere, address _revokeWho, bytes32 _revokePermission - ) = decodeGrantRevokeCalldata(_revokeData); + ) = _decodeGrantRevokeCalldata(_revokeData); if (_revokeSelector != PermissionManager.revoke.selector) return false; else if (!allowedPluginAddresses[_revokeWhere]) return false; @@ -162,8 +162,8 @@ contract OnlyPluginUpgraderCondition is PermissionCondition { return true; } - function isValidApplyUpdateCalldata(bytes memory _data) private view returns (bool) { - (bytes4 _selector, address _dao, address _targetPluginAddress) = decodeApplyUpdateCalldata( + function _isValidApplyUpdateCalldata(bytes memory _data) internal view returns (bool) { + (bytes4 _selector, address _dao, address _targetPluginAddress) = _decodeApplyUpdateCalldata( _data ); diff --git a/packages/contracts/src/test/TestMemberAccessExecuteCondition.sol b/packages/contracts/src/test/TestMemberAccessExecuteCondition.sol new file mode 100644 index 0000000..9f0a4be --- /dev/null +++ b/packages/contracts/src/test/TestMemberAccessExecuteCondition.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity 0.8.17; + +import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; +import {MemberAccessExecuteCondition} from "../conditions/MemberAccessExecuteCondition.sol"; + +/// @notice The condition associated with `TestSharedPlugin` +contract TestMemberAccessExecuteCondition is MemberAccessExecuteCondition { + constructor(address _targetContract) MemberAccessExecuteCondition(_targetContract) {} + + function getSelector(bytes memory _data) public pure returns (bytes4 selector) { + return super._getSelector(_data); + } + + function decodeAddRemoveMemberCalldata( + bytes memory _data + ) public pure returns (bytes4 sig, address account) { + return super._decodeAddRemoveMemberCalldata(_data); + } +} diff --git a/packages/contracts/src/test/TestOnlyPluginUpgraderCondition.sol b/packages/contracts/src/test/TestOnlyPluginUpgraderCondition.sol new file mode 100644 index 0000000..8c399b5 --- /dev/null +++ b/packages/contracts/src/test/TestOnlyPluginUpgraderCondition.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity 0.8.17; + +import {DAO} from "@aragon/osx/core/dao/DAO.sol"; +import {PluginSetupProcessor} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessor.sol"; +import {OnlyPluginUpgraderCondition} from "../conditions/OnlyPluginUpgraderCondition.sol"; + +/// @notice The condition associated with `TestSharedPlugin` +contract TestOnlyPluginUpgraderCondition is OnlyPluginUpgraderCondition { + constructor( + DAO _dao, + PluginSetupProcessor _psp, + address[] memory _targetPluginAddresses + ) OnlyPluginUpgraderCondition(_dao, _psp, _targetPluginAddresses) {} + + function getSelector(bytes memory _data) public pure returns (bytes4 selector) { + return super._getSelector(_data); + } + + function decodeGrantRevokeCalldata( + bytes memory _data + ) public pure returns (bytes4 selector, address where, address who, bytes32 permissionId) { + return super._decodeGrantRevokeCalldata(_data); + } + + function decodeApplyUpdateCalldata( + bytes memory _data + ) public pure returns (bytes4 selector, address daoAddress, address targetPluginAddress) { + return super._decodeApplyUpdateCalldata(_data); + } + + function isValidGrantRevokeCalldata( + bytes memory _grantData, + bytes memory _revokeData + ) public view returns (bool) { + return super._isValidGrantRevokeCalldata(_grantData, _revokeData); + } + + function isValidApplyUpdateCalldata(bytes memory _data) public view returns (bool) { + return super._isValidApplyUpdateCalldata(_data); + } +} 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 5eaae98..6a17383 100644 --- a/packages/contracts/test/unit-testing/member-access-execute-condition.ts +++ b/packages/contracts/test/unit-testing/member-access-execute-condition.ts @@ -5,6 +5,8 @@ import { MainVotingPlugin__factory, MemberAccessExecuteCondition, MemberAccessExecuteCondition__factory, + TestMemberAccessExecuteCondition__factory, + TestMemberAccessExecuteCondition, } from '../../typechain'; import {getPluginSetupProcessorAddress} from '../../utils/helpers'; import {deployTestDao} from '../helpers/test-dao'; @@ -313,7 +315,16 @@ describe('Member Access Condition', function () { }); }); - describe('Decoders', () => { + describe('Decoders (internal)', () => { + let testMemberAccessExecuteCondition: TestMemberAccessExecuteCondition; + + beforeEach(async () => { + const factory = new TestMemberAccessExecuteCondition__factory(alice); + testMemberAccessExecuteCondition = await factory.deploy( + SOME_CONTRACT_ADDRESS + ); + }); + it('Should decode getSelector properly', async () => { const actions: IDAO.ActionStruct[] = [ { @@ -333,15 +344,20 @@ describe('Member Access Condition', function () { ]; expect( - await memberAccessExecuteCondition.getSelector(actions[0].data) + await testMemberAccessExecuteCondition.getSelector(actions[0].data) ).to.eq((actions[0].data as string).slice(0, 10)); expect( - await memberAccessExecuteCondition.getSelector(actions[1].data) + await testMemberAccessExecuteCondition.getSelector(actions[1].data) ).to.eq((actions[1].data as string).slice(0, 10)); }); it('Should decode decodeGrantRevokeCalldata properly', async () => { + const factory = new TestMemberAccessExecuteCondition__factory(alice); + const testMemberAccessExecuteCondition = await factory.deploy( + SOME_CONTRACT_ADDRESS + ); + const calldataList = [ mainVotingPluginInterface.encodeFunctionData('addMember', [pspAddress]), mainVotingPluginInterface.encodeFunctionData('removeMember', [ @@ -351,7 +367,7 @@ describe('Member Access Condition', function () { // 1 let [selector, who] = - await memberAccessExecuteCondition.decodeAddRemoveMemberCalldata( + await testMemberAccessExecuteCondition.decodeAddRemoveMemberCalldata( calldataList[0] ); expect(selector).to.eq(calldataList[0].slice(0, 10)); @@ -359,7 +375,7 @@ describe('Member Access Condition', function () { // 2 [selector, who] = - await memberAccessExecuteCondition.decodeAddRemoveMemberCalldata( + await testMemberAccessExecuteCondition.decodeAddRemoveMemberCalldata( calldataList[1] ); expect(selector).to.eq(calldataList[1].slice(0, 10)); diff --git a/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts b/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts index 96fa8a9..5359cb3 100644 --- a/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts +++ b/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts @@ -6,6 +6,8 @@ import { IDAO, OnlyPluginUpgraderCondition, OnlyPluginUpgraderCondition__factory, + TestOnlyPluginUpgraderCondition__factory, + TestOnlyPluginUpgraderCondition, } from '../../typechain'; import {getPluginSetupProcessorAddress} from '../../utils/helpers'; import {deployTestDao} from '../helpers/test-dao'; @@ -1192,7 +1194,18 @@ describe('Only Plugin Upgrader Condition', function () { }); }); - describe('Decoders', () => { + describe('Decoders (internal)', () => { + let testMemberAccessExecuteCondition: TestOnlyPluginUpgraderCondition; + + beforeEach(async () => { + const factory = new TestOnlyPluginUpgraderCondition__factory(alice); + testMemberAccessExecuteCondition = await factory.deploy( + dao.address, + pspAddress, + [ALLOWED_PLUGIN_ADDRESS_1, ALLOWED_PLUGIN_ADDRESS_2] + ); + }); + it('Should decode getSelector properly', async () => { const actions: IDAO.ActionStruct[] = [ { @@ -1224,15 +1237,15 @@ describe('Only Plugin Upgrader Condition', function () { ]; expect( - await onlyPluginUpgraderCondition.getSelector(actions[0].data) + await testMemberAccessExecuteCondition.getSelector(actions[0].data) ).to.eq((actions[0].data as string).slice(0, 10)); expect( - await onlyPluginUpgraderCondition.getSelector(actions[1].data) + await testMemberAccessExecuteCondition.getSelector(actions[1].data) ).to.eq((actions[1].data as string).slice(0, 10)); expect( - await onlyPluginUpgraderCondition.getSelector(actions[2].data) + await testMemberAccessExecuteCondition.getSelector(actions[2].data) ).to.eq((actions[2].data as string).slice(0, 10)); }); @@ -1252,7 +1265,7 @@ describe('Only Plugin Upgrader Condition', function () { // 1 let [selector, where, who, permissionId] = - await onlyPluginUpgraderCondition.decodeGrantRevokeCalldata( + await testMemberAccessExecuteCondition.decodeGrantRevokeCalldata( calldataList[0] ); expect(selector).to.eq(calldataList[0].slice(0, 10)); @@ -1262,7 +1275,7 @@ describe('Only Plugin Upgrader Condition', function () { // 2 [selector, where, who, permissionId] = - await onlyPluginUpgraderCondition.decodeGrantRevokeCalldata( + await testMemberAccessExecuteCondition.decodeGrantRevokeCalldata( calldataList[1] ); expect(selector).to.eq(calldataList[1].slice(0, 10)); @@ -1289,7 +1302,7 @@ describe('Only Plugin Upgrader Condition', function () { // 1 let [selector, decodedDaoAddress, pluginAddress] = - await onlyPluginUpgraderCondition.decodeApplyUpdateCalldata( + await testMemberAccessExecuteCondition.decodeApplyUpdateCalldata( calldataList[0] ); expect(selector).to.eq(calldataList[0].slice(0, 10)); @@ -1298,7 +1311,7 @@ describe('Only Plugin Upgrader Condition', function () { // 2 [selector, decodedDaoAddress, pluginAddress] = - await onlyPluginUpgraderCondition.decodeApplyUpdateCalldata( + await testMemberAccessExecuteCondition.decodeApplyUpdateCalldata( calldataList[1] ); expect(selector).to.eq(calldataList[1].slice(0, 10));