diff --git a/contracts/LPFactory.sol b/contracts/LPFactory.sol index d7df7ff..9b57b9b 100644 --- a/contracts/LPFactory.sol +++ b/contracts/LPFactory.sol @@ -43,6 +43,7 @@ contract LPFactory is DAOFactory { bytes32 hatchCallerRole = v.ESCAPE_HATCH_CALLER_ROLE(); bytes32 authPaymentRole = v.AUTHORIZE_PAYMENT_ROLE(); bytes32 pledgeAdminRole = lp.PLEDGE_ADMIN_ROLE(); + bytes32 donorRole = lp.DONOR_ROLE(); bytes32 pluginManagerRole = lp.PLUGIN_MANAGER_ROLE(); acl.createPermission(_root, address(v), hatchCallerRole, _root); @@ -50,6 +51,7 @@ contract LPFactory is DAOFactory { acl.createPermission(_root, address(lp), pluginManagerRole, _root); acl.createPermission(address(lp), address(v), authPaymentRole, _root); acl.createPermission(0x0, address(lp), pledgeAdminRole, address(lp)); + acl.createPermission(0x0, address(lp), donorRole, address(lp)); // TODO: set pledgeAdminRole manager to 0x0? maybe it doesn't matter b/c it can be recreated by _root anyways acl.grantPermission(_root, address(kernel), appManagerRole); diff --git a/contracts/LPVault.sol b/contracts/LPVault.sol index 7c95200..08776d8 100644 --- a/contracts/LPVault.sol +++ b/contracts/LPVault.sol @@ -25,6 +25,7 @@ pragma solidity ^0.4.18; /// to allow for an optional escapeHatch to be implemented in case of issues; /// future versions of this contract will be enabled for tokens import "./EscapableApp.sol"; +import "./LiquidPledgingACLHelpers.sol"; import "giveth-common-contracts/contracts/ERC20.sol"; /// @dev `LiquidPledging` is a basic interface to allow the `LPVault` contract @@ -36,7 +37,7 @@ contract ILiquidPledging { /// @dev `LPVault` is a higher level contract built off of the `Escapable` /// contract that holds funds for the liquid pledging system. -contract LPVault is EscapableApp { +contract LPVault is EscapableApp, LiquidPledgingACLHelpers { bytes32 constant public CONFIRM_PAYMENT_ROLE = keccak256("CONFIRM_PAYMENT_ROLE"); bytes32 constant public CANCEL_PAYMENT_ROLE = keccak256("CANCEL_PAYMENT_ROLE"); @@ -107,7 +108,7 @@ contract LPVault is EscapableApp { /// @param _automatic If true, payments will confirm instantly, if false /// the training wheels are put on and the owner must manually approve /// every payment - function setAutopay(bool _automatic) external authP(SET_AUTOPAY_ROLE, _arr(_automatic)) { + function setAutopay(bool _automatic) external authP(SET_AUTOPAY_ROLE, arr(_automatic)) { autoPay = _automatic; AutoPaySet(autoPay); } @@ -225,13 +226,4 @@ contract LPVault is EscapableApp { CancelPayment(_idPayment, p.ref); } - - function _arr(bool a) internal pure returns (uint256[] r) { - r = new uint256[](1); - uint _a; - assembly { - _a := a // forced casting - } - r[0] = _a; - } } diff --git a/contracts/LiquidPledging.sol b/contracts/LiquidPledging.sol index 577362d..ed0c1fc 100644 --- a/contracts/LiquidPledging.sol +++ b/contracts/LiquidPledging.sol @@ -27,6 +27,7 @@ import "./LiquidPledgingBase.sol"; /// to allow for expanded functionality. contract LiquidPledging is LiquidPledgingBase { + function addGiverAndDonate(uint64 idReceiver, address token, uint amount) public { @@ -38,7 +39,7 @@ contract LiquidPledging is LiquidPledgingBase { { require(donorAddress != 0); // default to a 3 day (259200 seconds) commitTime - uint64 idGiver = _addGiver(donorAddress, "", "", 259200, ILiquidPledgingPlugin(0)); + uint64 idGiver = addGiver(donorAddress, "", "", 259200, ILiquidPledgingPlugin(0)); donate(idGiver, idReceiver, token, amount); } @@ -51,7 +52,7 @@ contract LiquidPledging is LiquidPledgingBase { /// @param idReceiver The Admin receiving the donation; can be any Admin: /// the Giver themselves, another Giver, a Delegate or a Project function donate(uint64 idGiver, uint64 idReceiver, address token, uint amount) - public + public authP(DONOR_ROLE, arr(idGiver, idReceiver, token, amount, msg.sender)) { require(idGiver > 0); // prevent burning donations. idReceiver is checked in _transfer require(amount > 0); diff --git a/contracts/LiquidPledgingACLHelpers.sol b/contracts/LiquidPledgingACLHelpers.sol new file mode 100644 index 0000000..b91f7ac --- /dev/null +++ b/contracts/LiquidPledgingACLHelpers.sol @@ -0,0 +1,21 @@ +pragma solidity ^0.4.18; + +contract LiquidPledgingACLHelpers { + function arr(uint64 a, uint64 b, address c, uint d, address e) internal pure returns(uint[] r) { + r = new uint[](4); + r[0] = uint(a); + r[1] = uint(b); + r[2] = uint(c); + r[3] = d; + r[4] = uint(e); + } + + function arr(bool a) internal pure returns (uint[] r) { + r = new uint[](1); + uint _a; + assembly { + _a := a // forced casting + } + r[0] = _a; + } +} \ No newline at end of file diff --git a/contracts/LiquidPledgingPlugins.sol b/contracts/LiquidPledgingPlugins.sol index 0916b92..764a4b6 100644 --- a/contracts/LiquidPledgingPlugins.sol +++ b/contracts/LiquidPledgingPlugins.sol @@ -21,8 +21,9 @@ pragma solidity ^0.4.18; import "@aragon/os/contracts/apps/AragonApp.sol"; import "./LiquidPledgingStorage.sol"; +import "./LiquidPledgingACLHelpers.sol"; -contract LiquidPledgingPlugins is AragonApp, LiquidPledgingStorage { +contract LiquidPledgingPlugins is AragonApp, LiquidPledgingStorage, LiquidPledgingACLHelpers { bytes32 constant public PLUGIN_MANAGER_ROLE = keccak256("PLUGIN_MANAGER_ROLE"); @@ -40,7 +41,7 @@ contract LiquidPledgingPlugins is AragonApp, LiquidPledgingStorage { pluginWhitelist[contractHash] = false; } - function useWhitelist(bool useWhitelist) external authP(PLUGIN_MANAGER_ROLE, _arr(useWhitelist)) { + function useWhitelist(bool useWhitelist) external authP(PLUGIN_MANAGER_ROLE, arr(useWhitelist)) { whitelistDisabled = !useWhitelist; } @@ -68,13 +69,4 @@ contract LiquidPledgingPlugins is AragonApp, LiquidPledgingStorage { } return keccak256(o_code); } - - function _arr(bool a) internal pure returns (uint[] r) { - r = new uint[](1); - uint _a; - assembly { - _a := a // forced casting - } - r[0] = _a; - } } \ No newline at end of file diff --git a/contracts/PledgeAdmins.sol b/contracts/PledgeAdmins.sol index 09ff9b6..c120353 100644 --- a/contracts/PledgeAdmins.sol +++ b/contracts/PledgeAdmins.sol @@ -24,7 +24,11 @@ import "@aragon/os/contracts/acl/ACL.sol"; contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { + // NOTE: PLEDGE_ADMIN_ROLE assumes that the 1st param passed to the authP modifier + // is the idAdmin. This is critical to prevent unauthorized access bytes32 constant public PLEDGE_ADMIN_ROLE = keccak256("PLEDGE_ADMIN_ROLE"); + bytes32 constant public DONOR_ROLE = keccak256("DONOR_ROLE"); + address constant public ANY_ENTITY = address(-1); // Limits inserted to prevent large loops that could prevent canceling uint constant MAX_SUBPROJECT_LEVEL = 20; @@ -57,7 +61,7 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { ILiquidPledgingPlugin plugin ) public returns (uint64 idGiver) { - return _addGiver( + return addGiver( msg.sender, name, url, @@ -66,7 +70,8 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { ); } - function _addGiver( + // TODO: is there an issue w/ allowing anyone to create a giver on behalf of another addy? + function addGiver( address addr, string name, string url, @@ -91,6 +96,7 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { url) ); + // TODO: do we want to grant permission accept donations as a giver? maybe from self only? _grantPledgeAdminPermission(msg.sender, idGiver); if (address(plugin) != 0) { _grantPledgeAdminPermission(address(plugin), idGiver); @@ -159,6 +165,7 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { url) ); + _grantAnyDonorPermission(idDelegate); _grantPledgeAdminPermission(msg.sender, idDelegate); if (address(plugin) != 0) { _grantPledgeAdminPermission(address(plugin), idDelegate); @@ -239,6 +246,7 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { url) ); + _grantAnyDonorPermission(idProject); _grantPledgeAdminPermission(projectAdmin, idProject); if (address(plugin) != 0) { _grantPledgeAdminPermission(address(plugin), idProject); @@ -277,6 +285,32 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { ProjectUpdated(idProject); } + function grantPledgeAdminPermission(uint64 idAdmin, address _who, uint[] _params) public authP(PLEDGE_ADMIN_ROLE, arr(uint(idAdmin), 0, 1)) { + uint[] memory params; + + // if params are passed, we need to add a AND logic statement as the first param + // which limits the permission to the given idAdmin. This is to prevent granting + // PledgeAdminPermission to a idAdmin that msg.sender is not an admin to + // idAdmin (on msg call) == idAdmin AND _params + if (_params.length > 0) { + params = new uint[](_params.length + 2); + // paramId: 204 (LOGIC) op: AND(8) val: 1 (param index) & 2 (param index) + params[0] = uint(bytes32(204 << 8 * 31) | bytes32(8 << 8 * 30) | bytes32(2 << 8 * 4) | bytes32(1)); + // paramId: 0 op: EQ(1) val: idAdmin + params[1] = uint(bytes32(1 << 8 * 30) | idAdmin); + + for (uint64 i = 0; i < _params.length; i++) { + params[i + 2] = _params[i]; + } + } else { + params = new uint[](1); + // paramId: 0 op: EQ(1) val: idAdmin + params[0] = uint(bytes32(1 << 8 * 30) | idAdmin); + } + + ACL(kernel.acl()).grantPermissionP(_who, address(this), PLEDGE_ADMIN_ROLE, _params); + } + ///////////////////////////// // Public constant functions @@ -374,13 +408,27 @@ contract PledgeAdmins is AragonApp, LiquidPledgingPlugins { return _getProjectLevel(parent) + 1; } - function _grantPledgeAdminPermission(address _who, uint64 idPledge) internal { + function _grantPledgeAdminPermission(address _who, uint64 idAdmin) internal { bytes32 id; - assembly { id := idPledge } + assembly { id := idAdmin } uint[] memory params = new uint[](1); + // paramId: 0 op: EQ(1) val: idAdmin params[0] = uint(bytes32(1 << 8 * 30) | id); + // grant _who the PLEDGE_ADMIN_ROLE for idAdmin ACL(kernel.acl()).grantPermissionP(_who, address(this), PLEDGE_ADMIN_ROLE, params); } + + function _grantAnyDonorPermission(uint64 idAdmin) internal { + bytes32 id; + assembly { id := idAdmin } + + uint[] memory params = new uint[](1); + // paramId: 1 op: EQ(1) val: idAdmin + params[0] = uint(bytes32(1 << 8 * 31) | bytes32(1 << 8 * 30) | id); + + // grant ANY_ENTITY permission to donate to idAdmin + ACL(kernel.acl()).grantPermissionP(ANY_ENTITY, address(this), DONOR_ROLE, params); + } } \ No newline at end of file diff --git a/test/NormalOperation.js b/test/NormalOperation.js index 25d821c..6cc0c9c 100644 --- a/test/NormalOperation.js +++ b/test/NormalOperation.js @@ -68,7 +68,7 @@ describe('LiquidPledging test', function () { it('Should deploy LiquidPledging contract', async () => { const baseVault = await contracts.LPVault.new(web3); - const baseLP = await contracts.LiquidPledgingMock.new(web3); + const baseLP = await contracts.LiquidPledgingMock.new(web3, {gas: 6700000}); lpFactory = await contracts.LPFactory.new(web3, baseVault.$address, baseLP.$address); const r = await lpFactory.newLP(accounts[0], escapeHatchDestination); @@ -100,6 +100,11 @@ describe('LiquidPledging test', function () { }); it('Should create a giver', async () => { await liquidPledging.addGiver('Giver1', 'URLGiver1', 86400, 0, { from: giver1, gas: 1000000 }); + + // grant permission to giver1 to receive donations. This is not done by default in liquidPledging, + // but the test cases use this. + await acl.grantPermissionP(await liquidPledging.ANY_ENTITY(), liquidPledging.$address, await liquidPledging.DONOR_ROLE(), ["0x101000000000000000000000000000000000000000000000000000000000000"], {$extraGas: 200000}); + const nAdmins = await liquidPledging.numberOfPledgeAdmins(); assert.equal(nAdmins, 1); const res = await liquidPledging.getPledgeAdmin(1);