diff --git a/contracts/colony/ColonyExpenditure.sol b/contracts/colony/ColonyExpenditure.sol index ac639d3e5c..a464e86f47 100644 --- a/contracts/colony/ColonyExpenditure.sol +++ b/contracts/colony/ColonyExpenditure.sol @@ -214,6 +214,7 @@ contract ColonyExpenditure is ColonyStorage { require(_slots.length == _payoutModifiers.length, "colony-expenditure-bad-slots"); for (uint256 i; i < _slots.length; i++) { + require(_payoutModifiers[i] <= 0, "colony-expenditure-bad-payout-modifier"); expenditureSlots[_id][_slots[i]].payoutModifier = _payoutModifiers[i]; emit ExpenditurePayoutModifierSet(msgSender(), _id, _slots[i], _payoutModifiers[i]); @@ -315,15 +316,14 @@ contract ColonyExpenditure is ColonyStorage { // Validate payout modifier if (offset == 2) { - require( - int256(uint256(_value)) <= MAX_PAYOUT_MODIFIER && - int256(uint256(_value)) >= MIN_PAYOUT_MODIFIER, - "colony-expenditure-bad-payout-modifier" - ); + if (!ColonyAuthority(address(authority)).hasUserRole(msgSender(), 1, uint8(ColonyDataTypes.ColonyRole.Root))){ + require(int256(uint256(_value)) <= 0, "colony-expenditure-bad-payout-modifier"); + } + require(int256(uint256(_value)) >= MIN_PAYOUT_MODIFIER, "colony-expenditure-bad-payout-modifier"); } } else { - require(false, "colony-expenditure-bad-slot"); + revert("colony-expenditure-bad-slot"); } executeStateChange(keccak256(abi.encode(_id, _storageSlot)), _mask, _keys, _value); diff --git a/contracts/colony/ColonyFunding.sol b/contracts/colony/ColonyFunding.sol index 79cf94385b..35212f1c23 100755 --- a/contracts/colony/ColonyFunding.sol +++ b/contracts/colony/ColonyFunding.sol @@ -209,7 +209,7 @@ contract ColonyFunding is ColonyStorage { // ignore-swc-123 uint256 initialPayout = expenditureSlotPayouts[_id][_slot][_token]; delete expenditureSlotPayouts[_id][_slot][_token]; - int256 payoutModifier = imin(imax(slot.payoutModifier, MIN_PAYOUT_MODIFIER), MAX_PAYOUT_MODIFIER); + int256 payoutModifier = imax(slot.payoutModifier, MIN_PAYOUT_MODIFIER); uint256 payoutScalar = uint256(payoutModifier + int256(WAD)); uint256 repPayout = wmul(initialPayout, payoutScalar); diff --git a/contracts/colonyNetwork/ColonyNetworkENS.sol b/contracts/colonyNetwork/ColonyNetworkENS.sol index f77f6e53af..748e50f509 100644 --- a/contracts/colonyNetwork/ColonyNetworkENS.sol +++ b/contracts/colonyNetwork/ColonyNetworkENS.sol @@ -152,6 +152,6 @@ contract ColonyNetworkENS is ColonyNetworkStorage, MultiChain { } else if (isXdai()) { return "joincolony.colonyxdai"; } - require(false, "colony-network-unsupported-network"); + revert("colony-network-unsupported-network"); } } diff --git a/contracts/extensions/EvaluatedExpenditure.sol b/contracts/extensions/EvaluatedExpenditure.sol index b00b0d43f8..25631f0130 100644 --- a/contracts/extensions/EvaluatedExpenditure.sol +++ b/contracts/extensions/EvaluatedExpenditure.sol @@ -82,7 +82,7 @@ contract EvaluatedExpenditure is ColonyExtension, BasicMetaTransaction { /// @param _childSkillIndex The index that the `_domainId` is relative to `_permissionDomainId` /// @param _id Expenditure identifier /// @param _slots Array of slots to set payout modifiers - /// @param _payoutModifiers Values (between +/- WAD) to modify the payout & reputation bonus + /// @param _payoutModifiers Values (between -WAD and 0) to modify the payout & reputation bonus function setExpenditurePayoutModifiers( uint256 _permissionDomainId, uint256 _childSkillIndex, diff --git a/contracts/testHelpers/RequireExecuteCall.sol b/contracts/testHelpers/RequireExecuteCall.sol index 383c4efba4..bca90267bf 100644 --- a/contracts/testHelpers/RequireExecuteCall.sol +++ b/contracts/testHelpers/RequireExecuteCall.sol @@ -33,9 +33,9 @@ contract RequireExecuteCall { // Slice the sighash. returndata := add(returndata, 0x04) } - require(false, abi.decode(returndata, (string))); // All that remains is the revert string + revert(abi.decode(returndata, (string))); // All that remains is the revert string } - require(false, "require-execute-call-reverted-with-no-error"); + revert("require-execute-call-reverted-with-no-error"); } } } \ No newline at end of file diff --git a/docs/interfaces/extensions/evaluatedexpenditure.md b/docs/interfaces/extensions/evaluatedexpenditure.md index c4d2c244a3..f0c3865f91 100644 --- a/docs/interfaces/extensions/evaluatedexpenditure.md +++ b/docs/interfaces/extensions/evaluatedexpenditure.md @@ -82,7 +82,7 @@ Sets the payout modifiers in given expenditure slots, using the arbitration perm |_childSkillIndex|uint256|The index that the `_domainId` is relative to `_permissionDomainId` |_id|uint256|Expenditure identifier |_slots|uint256[]|Array of slots to set payout modifiers -|_payoutModifiers|int256[]|Values (between +/- WAD) to modify the payout & reputation bonus +|_payoutModifiers|int256[]|Values (between -WAD and 0) to modify the payout & reputation bonus ### ▸ `uninstall()` diff --git a/helpers/test-helper.js b/helpers/test-helper.js index ae41ac4a41..05941f7b44 100644 --- a/helpers/test-helper.js +++ b/helpers/test-helper.js @@ -1113,7 +1113,7 @@ exports.getRewardClaimSquareRootsAndProofs = async function getRewardClaimSquare }; exports.bn2bytes32 = function bn2bytes32(x, size = 64) { - return `0x${x.toString(16, size)}`; + return `0x${x.toTwos(size * 4).toString(16, size)}`; }; exports.rolesToBytes32 = function rolesToBytes32(roles) { diff --git a/test/contracts-network/colony-expenditure.js b/test/contracts-network/colony-expenditure.js index d739aa9ea5..ce233d9336 100644 --- a/test/contracts-network/colony-expenditure.js +++ b/test/contracts-network/colony-expenditure.js @@ -340,14 +340,21 @@ contract("Colony Expenditure", (accounts) => { "colony-expenditure-not-owner" ); - await colony.setExpenditurePayoutModifiers(expenditureId, [SLOT1, SLOT2], [WAD.divn(2), WAD], { from: ADMIN }); + await colony.setExpenditurePayoutModifiers(expenditureId, [SLOT1, SLOT2], [WAD.divn(-2), WAD.muln(-1)], { from: ADMIN }); expenditureSlot = await colony.getExpenditureSlot(expenditureId, SLOT0); expect(expenditureSlot.payoutModifier).to.be.zero; expenditureSlot = await colony.getExpenditureSlot(expenditureId, SLOT1); - expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.divn(2)); + expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.divn(-2)); expenditureSlot = await colony.getExpenditureSlot(expenditureId, SLOT2); - expect(expenditureSlot.payoutModifier).to.eq.BN(WAD); + expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.muln(-1)); + }); + + it("should not allow owners to update many slot payout modifiers at once if one is >0", async () => { + await checkErrorRevert( + colony.setExpenditurePayoutModifiers(expenditureId, [SLOT1, SLOT2], [0, 1], { from: ADMIN }), + "colony-expenditure-bad-payout-modifier" + ); }); it("should not allow owners to update many slot payout modifiers with mismatched arguments", async () => { @@ -463,7 +470,7 @@ contract("Colony Expenditure", (accounts) => { [SLOT0, SLOT1], [10, 20], [SLOT0, SLOT2], - [WAD.divn(3), WAD.divn(2)], + [WAD.divn(-3), WAD.divn(-2)], [token.address, otherToken.address], [ [SLOT0, SLOT1], @@ -481,7 +488,7 @@ contract("Colony Expenditure", (accounts) => { expect(slot.recipient).to.equal(RECIPIENT); expect(slot.skills[0]).to.be.zero; expect(slot.claimDelay).to.eq.BN(10); - expect(slot.payoutModifier).to.eq.BN(WAD.divn(3)); + expect(slot.payoutModifier).to.eq.BN(WAD.divn(-3)); slot = await colony.getExpenditureSlot(expenditureId, SLOT1); expect(slot.recipient).to.equal(USER); @@ -493,7 +500,7 @@ contract("Colony Expenditure", (accounts) => { expect(slot.recipient).to.equal(ADMIN); expect(slot.skills[0]).to.eq.BN(GLOBAL_SKILL_ID); expect(slot.claimDelay).to.be.zero; - expect(slot.payoutModifier).to.eq.BN(WAD.divn(2)); + expect(slot.payoutModifier).to.eq.BN(WAD.divn(-2)); let payout; payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT0, token.address); @@ -522,7 +529,7 @@ contract("Colony Expenditure", (accounts) => { [SLOT0, SLOT1], [10, 20], [SLOT0, SLOT2], - [WAD.divn(3), WAD.divn(2)], + [WAD.divn(-3), WAD.divn(-2)], [token.address, otherToken.address], [[SLOT0], [SLOT1, SLOT2]], [ @@ -561,7 +568,7 @@ contract("Colony Expenditure", (accounts) => { [SLOT0, SLOT1], [10, 20], [SLOT0, SLOT2], - [WAD.divn(3), WAD.divn(2)], + [WAD.divn(-3), WAD.divn(-2)], [token.address, otherToken.address], [ [SLOT0, SLOT1], @@ -582,7 +589,7 @@ contract("Colony Expenditure", (accounts) => { expect(slot.recipient).to.equal(RECIPIENT); expect(slot.skills[0]).to.be.zero; expect(slot.claimDelay).to.eq.BN(10); - expect(slot.payoutModifier).to.eq.BN(WAD.divn(3)); + expect(slot.payoutModifier).to.eq.BN(WAD.divn(-3)); slot = await colony.getExpenditureSlot(expenditureId, SLOT1); expect(slot.recipient).to.equal(USER); @@ -594,7 +601,7 @@ contract("Colony Expenditure", (accounts) => { expect(slot.recipient).to.equal(ADMIN); expect(slot.skills[0]).to.eq.BN(GLOBAL_SKILL_ID); expect(slot.claimDelay).to.be.zero; - expect(slot.payoutModifier).to.eq.BN(WAD.divn(2)); + expect(slot.payoutModifier).to.eq.BN(WAD.divn(-2)); let payout; payout = await colony.getExpenditureSlotPayout(expenditureId, SLOT0, token.address); @@ -1274,12 +1281,48 @@ contract("Colony Expenditure", (accounts) => { it("should allow arbitration users to update expenditure slot payoutModifier", async () => { const mask = [MAPPING, ARRAY]; const keys = ["0x0", bn2bytes32(new BN(2))]; - const value = bn2bytes32(new BN(100)); + const value = bn2bytes32(new BN(100).muln(-1)); await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ARBITRATOR }); const expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); - expect(expenditureSlot.payoutModifier).to.eq.BN(100); + expect(expenditureSlot.payoutModifier).to.eq.BN(new BN(100).muln(-1)); + }); + + it("should allow not allow arbitration users to update expenditure slot payoutModifier to a value greater than 1", async () => { + const mask = [MAPPING, ARRAY]; + const keys = ["0x0", bn2bytes32(new BN(2))]; + const value = bn2bytes32(WAD.muln(2)); + + await checkErrorRevert( + colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ARBITRATOR }), + "colony-expenditure-bad-payout-modifier" + ); + + const expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); + expect(expenditureSlot.payoutModifier).to.eq.BN(0); + }); + + it("should allow root users to update expenditure slot payoutModifier to a value greater than 1", async () => { + const mask = [MAPPING, ARRAY]; + const keys = ["0x0", bn2bytes32(new BN(2))]; + const value = bn2bytes32(WAD.muln(2)); + + await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ROOT }); + + const expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); + expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.muln(2)); + }); + + it("not even root users should be allowed to update expenditure slot payoutModifier to a value less than -1", async () => { + const mask = [MAPPING, ARRAY]; + const keys = ["0x0", bn2bytes32(new BN(2))]; + const value = bn2bytes32(WAD.muln(-2)); + + await checkErrorRevert( + colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ROOT }), + "colony-expenditure-bad-payout-modifier" + ); }); it("should not allow arbitration users to pass an invalid payoutModifier", async () => { @@ -1451,7 +1494,7 @@ contract("Colony Expenditure", (accounts) => { const mask = [MAPPING, ARRAY]; const keys = ["0x0", bn2bytes32(new BN(2))]; const value = bn2bytes32(WAD); - await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ARBITRATOR }); + await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ROOT }); const expenditure = await colony.getExpenditure(expenditureId); await colony.moveFundsBetweenPots(1, UINT256_MAX, UINT256_MAX, domain1.fundingPotId, expenditure.fundingPotId, WAD, token.address); @@ -1473,14 +1516,44 @@ contract("Colony Expenditure", (accounts) => { expect(entry.amount).to.eq.BN(WAD.muln(2)); }); + it("should scale up payout by payoutScalar for large payouts", async () => { + await colony.setExpenditureRecipient(expenditureId, SLOT0, RECIPIENT, { from: ADMIN }); + await colony.setExpenditurePayout(expenditureId, SLOT0, token.address, WAD, { from: ADMIN }); + + // Modifier of 1 WAD translates to scalar of 2 WAD + const mask = [MAPPING, ARRAY]; + const keys = ["0x0", bn2bytes32(new BN(2))]; + const value = bn2bytes32(WAD.muln(100)); + await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ROOT }); + + const expenditure = await colony.getExpenditure(expenditureId); + await colony.moveFundsBetweenPots(1, UINT256_MAX, UINT256_MAX, domain1.fundingPotId, expenditure.fundingPotId, WAD, token.address); + await colony.finalizeExpenditure(expenditureId, { from: ADMIN }); + + const recipientBalanceBefore = await token.balanceOf(RECIPIENT); + await colony.claimExpenditurePayout(expenditureId, SLOT0, token.address); + + // Cash payout maxes out at payout + const recipientBalanceAfter = await token.balanceOf(RECIPIENT); + expect(recipientBalanceAfter.sub(recipientBalanceBefore)).to.eq.BN(WAD.divn(100).muln(99).subn(1)); // eslint-disable-line prettier/prettier + + // But reputation gets a boost + const addr = await colonyNetwork.getReputationMiningCycle(false); + const repCycle = await IReputationMiningCycle.at(addr); + const numEntries = await repCycle.getReputationUpdateLogLength(); + const entry = await repCycle.getReputationUpdateLogEntry(numEntries.subn(1)); + expect(entry.user).to.equal(RECIPIENT); + expect(entry.amount).to.eq.BN(WAD.muln(101)); + }); + it("should not overflow when using the maximum payout * modifier", async () => { await colony.setExpenditureRecipient(expenditureId, SLOT0, RECIPIENT, { from: ADMIN }); await colony.setExpenditurePayout(expenditureId, SLOT0, token.address, MAX_PAYOUT, { from: ADMIN }); const mask = [MAPPING, ARRAY]; const keys = ["0x0", bn2bytes32(new BN(2))]; - const value = bn2bytes32(WAD); - await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ARBITRATOR }); + const value = bn2bytes32(UINT256_MAX); + await colony.setExpenditureState(1, UINT256_MAX, expenditureId, EXPENDITURESLOTS_SLOT, mask, keys, value, { from: ROOT }); const expenditure = await colony.getExpenditure(expenditureId); await colony.moveFundsBetweenPots(1, UINT256_MAX, UINT256_MAX, domain1.fundingPotId, expenditure.fundingPotId, MAX_PAYOUT, token.address); diff --git a/test/extensions/evaluated-expenditure.js b/test/extensions/evaluated-expenditure.js index 5975e30c8a..f2a8a4e66e 100644 --- a/test/extensions/evaluated-expenditure.js +++ b/test/extensions/evaluated-expenditure.js @@ -101,10 +101,10 @@ contract("EvaluatedExpenditure", (accounts) => { expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); expect(expenditureSlot.payoutModifier).to.be.zero; - await evaluatedExpenditure.setExpenditurePayoutModifiers(1, UINT256_MAX, expenditureId, [0], [WAD], { from: USER0 }); + await evaluatedExpenditure.setExpenditurePayoutModifiers(1, UINT256_MAX, expenditureId, [0], [WAD.muln(-1)], { from: USER0 }); expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); - expect(expenditureSlot.payoutModifier).to.eq.BN(WAD); + expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.muln(-1)); }); it("cannot set the payout modifier with bad arguments", async () => { @@ -123,7 +123,7 @@ contract("EvaluatedExpenditure", (accounts) => { it("can set the payout modifier via metatransaction", async () => { const txData = await evaluatedExpenditure.contract.methods - .setExpenditurePayoutModifiers(1, UINT256_MAX.toString(), expenditureId.toString(), [0], [WAD.toString()]) + .setExpenditurePayoutModifiers(1, UINT256_MAX.toString(), expenditureId.toString(), [0], [WAD.muln(-1)]) .encodeABI(); const { r, s, v } = await getMetaTransactionParameters(txData, USER0, evaluatedExpenditure.address); @@ -135,7 +135,7 @@ contract("EvaluatedExpenditure", (accounts) => { await evaluatedExpenditure.executeMetaTransaction(USER0, txData, r, s, v, { from: USER1 }); expenditureSlot = await colony.getExpenditureSlot(expenditureId, 0); - expect(expenditureSlot.payoutModifier).to.eq.BN(WAD); + expect(expenditureSlot.payoutModifier).to.eq.BN(WAD.muln(-1)); }); }); });