From 771989839063a5a4ec67c8870ef08e90bf381086 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Tue, 17 Jan 2023 11:13:12 -0300 Subject: [PATCH 01/24] refactor(contracts/erc20): dont call snapshot too often when mint/burn multiple in ERC20Snapshot --- contracts/utils/ERC20/ERC20SnapshotRep.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/ERC20/ERC20SnapshotRep.sol b/contracts/utils/ERC20/ERC20SnapshotRep.sol index db7f4c27..9c33e204 100644 --- a/contracts/utils/ERC20/ERC20SnapshotRep.sol +++ b/contracts/utils/ERC20/ERC20SnapshotRep.sol @@ -71,9 +71,9 @@ contract ERC20SnapshotRep is OwnableUpgradeable, ERC20SnapshotUpgradeable { for (uint256 i = 0; i < accounts.length; i++) { _addHolder(accounts[i]); _mint(accounts[i], amount[i]); - _snapshot(); emit Mint(accounts[i], amount[i]); } + _snapshot(); return true; } @@ -105,9 +105,9 @@ contract ERC20SnapshotRep is OwnableUpgradeable, ERC20SnapshotUpgradeable { for (uint256 i = 0; i < accounts.length; i++) { _burn(accounts[i], amount[i]); _removeHolder(accounts[i]); - _snapshot(); emit Burn(accounts[i], amount[i]); } + _snapshot(); return true; } From 5214bac2a623f4c83712e45a10f80de07b1efbec Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 18 Jan 2023 11:00:27 -0300 Subject: [PATCH 02/24] refactor(contracts/dao): small refactor on withdrawRefundBalance in voting machine and tests --- contracts/dao/votingMachine/VotingMachine.sol | 17 ++-- test/dao/votingMachines/VotingMachine.js | 98 +++++++++++++++++++ 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index e78daf26..298a2d2f 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -552,21 +552,20 @@ contract VotingMachine { /** * @dev Withdraw scheme refund balance + * @param avatar The avatar address of the dao that controls the scheme * @param scheme Scheme contract address to withdraw refund balance from */ - function withdrawRefundBalance(address scheme) external { - bytes32 schemeId = keccak256(abi.encodePacked(msg.sender, scheme)); - - if (schemes[schemeId].voteGas <= 0) { - revert VotingMachine__AddressNotRegisteredInSchemeRefounds(); + function withdrawRefundBalance(address avatar, address scheme) external { + bytes32 schemeId; + if (msg.sender == scheme) { + schemeId = keccak256(abi.encodePacked(msg.sender, avatar)); + } else if (msg.sender == avatar) { + schemeId = keccak256(abi.encodePacked(scheme, msg.sender)); } - if (schemes[schemeId].voteGasBalance <= 0) { - revert VotingMachine__SchemeRefundBalanceIsZero(); - } uint256 voteGasBalance = schemes[schemeId].voteGasBalance; schemes[schemeId].voteGasBalance = 0; - payable(msg.sender).transfer(voteGasBalance); + payable(avatar).transfer(voteGasBalance); } /** diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index 4a8e03b6..a7f794ee 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -249,6 +249,15 @@ contract("VotingMachine", function (accounts) { constants.MAX_UINT_256, true ); + await permissionRegistry.setETHPermission( + org.avatar.address, + dxdVotingMachine.address, + web3.eth.abi.encodeFunctionSignature( + "withdrawRefundBalance(address,address)" + ), + 0, + true + ); await permissionRegistry.setETHPermission( org.avatar.address, constants.ZERO_ADDRESS, @@ -400,6 +409,95 @@ contract("VotingMachine", function (accounts) { assert.equal(schemeProposal.value[0], 0); }); + it("pay for gasRefund from votingMachine and withdrawBalance after that", async function () { + const setRefundConfData = web3.eth.abi.encodeFunctionCall( + VotingMachine.abi.find(x => x.name === "setSchemeRefund"), + [ + org.avatar.address, + masterAvatarScheme.address, + VOTE_GAS, + constants.GAS_PRICE, + ] + ); + + const setRefundConfProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [dxdVotingMachine.address], + [setRefundConfData], + [TOTAL_GAS_REFUND_PER_VOTE.mul(new BN(6))], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + const schemeId = ( + await dxdVotingMachine.proposals(setRefundConfProposalId) + ).schemeId; + + await dxdVotingMachine.vote( + setRefundConfProposalId, + constants.YES_OPTION, + 0, + { from: accounts[3], gasPrice: constants.GAS_PRICE } + ); + + assert.equal( + TOTAL_GAS_REFUND_PER_VOTE * 5, + Number((await dxdVotingMachine.schemes(schemeId)).voteGasBalance) + ); + + const withdrawRefundBalanceData = web3.eth.abi.encodeFunctionCall( + VotingMachine.abi.find(x => x.name === "withdrawRefundBalance"), + [org.avatar.address, masterAvatarScheme.address] + ); + + let withdrawRefundBalanceProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [dxdVotingMachine.address], + [withdrawRefundBalanceData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + assert.equal( + TOTAL_GAS_REFUND_PER_VOTE * 5, + Number((await dxdVotingMachine.schemes(schemeId)).voteGasBalance) + ); + + await dxdVotingMachine.vote( + withdrawRefundBalanceProposalId, + constants.YES_OPTION, + 0, + { + from: accounts[3], + gasPrice: constants.GAS_PRICE, + gasLimit: constants.GAS_LIMIT, + } + ); + + const schemeProposal = await masterAvatarScheme.getProposal( + withdrawRefundBalanceProposalId + ); + assert.equal( + schemeProposal.state, + constants.WALLET_SCHEME_PROPOSAL_STATES.passed + ); + assert.equal( + (await dxdVotingMachine.proposals(withdrawRefundBalanceProposalId)) + .executionState, + constants.VOTING_MACHINE_EXECUTION_STATES.QueueBarCrossed + ); + + assert.equal( + 0, + Number((await dxdVotingMachine.schemes(schemeId)).voteGasBalance) + ); + }); + it("Can view rep of votes and amount staked on proposal", async function () { const statusInfo = await dxdVotingMachine.getProposalStatus( setRefundConfProposalId From 15dfede58b9a22aabe403b3d230416840f1d68a8 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 18 Jan 2023 11:01:21 -0300 Subject: [PATCH 03/24] fix(contracts/dao): _execute function in VotingMachine returns true if executed or not --- contracts/dao/votingMachine/VotingMachine.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 298a2d2f..238e3929 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -891,7 +891,7 @@ contract VotingMachine { if (tmpProposal.state != proposal.state) { emit StateChange(proposalId, proposal.state); } - return (proposal.executionState != ExecutionState.None && proposal.executionState != ExecutionState.Failed); + return (proposal.executionState != ExecutionState.None); } /** From eb930303584b70e9b40a850b909c5aa8dc83f6e0 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 18 Jan 2023 11:09:18 -0300 Subject: [PATCH 04/24] fix(contracts/dao): fix preBoostedProposalsCounter if max boosted proposals is reached --- contracts/dao/votingMachine/VotingMachine.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 238e3929..c4e0e87e 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -830,12 +830,13 @@ contract VotingMachine { // change proposal mode to Boosted mode. proposal.state = ProposalState.Boosted; proposal.times[1] = proposal.times[2] + params.preBoostedVotePeriodLimit; + schemes[proposal.schemeId].preBoostedProposalsCounter--; schemes[proposal.schemeId].boostedProposalsCounter++; } } else { proposal.state = ProposalState.Queued; + schemes[proposal.schemeId].preBoostedProposalsCounter--; } - schemes[proposal.schemeId].preBoostedProposalsCounter--; } else { // check the Confidence level is stable if (score(proposalId) <= getSchemeThreshold(proposal.paramsHash, proposal.schemeId)) { From e10149f53ff8d35698cff301de189183f252a5e8 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 18 Jan 2023 20:31:12 -0300 Subject: [PATCH 05/24] refactor(contracts/dao): remove unnecessaty isSchemeRegistered flag in DAOController --- contracts/dao/DAOController.sol | 45 ++++++-------------- test/dao/DAOController.js | 70 ++------------------------------ test/dao/schemes/WalletScheme.js | 12 ------ 3 files changed, 16 insertions(+), 111 deletions(-) diff --git a/contracts/dao/DAOController.sol b/contracts/dao/DAOController.sol index 0094561a..24ab1e11 100644 --- a/contracts/dao/DAOController.sol +++ b/contracts/dao/DAOController.sol @@ -17,7 +17,6 @@ contract DAOController is Initializable { struct Scheme { bytes32 paramsHash; // a hash voting parameters of the scheme - bool isRegistered; bool canManageSchemes; bool canMakeAvatarCalls; bool canChangeReputation; @@ -61,19 +60,15 @@ contract DAOController is Initializable { /// @notice Cannot unregister last scheme with manage schemes permission error DAOController__CannotUnregisterLastSchemeWithManageSchemesPermission(); + /// @notice Cannot register a scheme with paramsHash 0 + error DAOController__CannotRegisterSchemeWithNullParamsHash(); + /// @notice Sender is not the scheme that originally started the proposal error DAOController__SenderIsNotTheProposer(); /// @notice Sender is not a registered scheme or proposal is not active error DAOController__SenderIsNotRegisteredOrProposalIsInactive(); - /// @dev Verify if scheme is registered - modifier onlyRegisteredScheme() { - if (!schemes[msg.sender].isRegistered) { - revert DAOController__SenderNotRegistered(); - } - _; - } /// @dev Verify if scheme can manage schemes modifier onlyRegisteringSchemes() { if (!schemes[msg.sender].canManageSchemes) { @@ -111,7 +106,6 @@ contract DAOController is Initializable { ) public initializer { schemes[scheme] = Scheme({ paramsHash: paramsHash, - isRegistered: true, canManageSchemes: true, canMakeAvatarCalls: true, canChangeReputation: true @@ -135,11 +129,15 @@ contract DAOController is Initializable { bool canManageSchemes, bool canMakeAvatarCalls, bool canChangeReputation - ) external onlyRegisteredScheme onlyRegisteringSchemes returns (bool success) { + ) external onlyRegisteringSchemes returns (bool success) { Scheme memory scheme = schemes[schemeAddress]; + if (paramsHash == bytes32(0)) { + revert DAOController__CannotRegisterSchemeWithNullParamsHash(); + } + // Add or change the scheme: - if ((!scheme.isRegistered || !scheme.canManageSchemes) && canManageSchemes) { + if (!scheme.canManageSchemes && canManageSchemes) { schemesWithManageSchemesPermission = schemesWithManageSchemesPermission + 1; } else if (scheme.canManageSchemes && !canManageSchemes) { if (schemesWithManageSchemesPermission <= 1) { @@ -150,7 +148,6 @@ contract DAOController is Initializable { schemes[schemeAddress] = Scheme({ paramsHash: paramsHash, - isRegistered: true, canManageSchemes: canManageSchemes, canMakeAvatarCalls: canMakeAvatarCalls, canChangeReputation: canChangeReputation @@ -166,16 +163,11 @@ contract DAOController is Initializable { * @param schemeAddress The address of the scheme to unregister/delete from `schemes` mapping * @return success Success of the operation */ - function unregisterScheme(address schemeAddress) - external - onlyRegisteredScheme - onlyRegisteringSchemes - returns (bool success) - { + function unregisterScheme(address schemeAddress) external onlyRegisteringSchemes returns (bool success) { Scheme memory scheme = schemes[schemeAddress]; //check if the scheme is registered - if (_isSchemeRegistered(schemeAddress) == false) { + if (scheme.paramsHash == bytes32(0)) { return false; } @@ -206,7 +198,7 @@ contract DAOController is Initializable { bytes calldata data, DAOAvatar avatar, uint256 value - ) external onlyRegisteredScheme onlyAvatarCallScheme returns (bool callSuccess, bytes memory callData) { + ) external onlyAvatarCallScheme returns (bool callSuccess, bytes memory callData) { return avatar.executeCall(to, data, value); } @@ -243,15 +235,6 @@ contract DAOController is Initializable { reputationToken.transferOwnership(newOwner); } - /** - * @dev Returns whether a scheme is registered or not - * @param scheme The address of the scheme - * @return isRegistered Whether a scheme is registered or not - */ - function isSchemeRegistered(address scheme) external view returns (bool isRegistered) { - return _isSchemeRegistered(scheme); - } - /** * @dev Returns scheme paramsHash * @param scheme The address of the scheme @@ -300,10 +283,6 @@ contract DAOController is Initializable { return schemesWithManageSchemesPermission; } - function _isSchemeRegistered(address scheme) private view returns (bool) { - return (schemes[scheme].isRegistered); - } - /** * @dev Function to get reputation token * @return tokenAddress The reputation token set on controller.initialize diff --git a/test/dao/DAOController.js b/test/dao/DAOController.js index 5925cfa1..237cb36c 100644 --- a/test/dao/DAOController.js +++ b/test/dao/DAOController.js @@ -139,18 +139,17 @@ contract("DAOController", function (accounts) { ); }); - it('registerScheme() should reject with: "DAOController__SenderNotRegistered"', async function () { + it('registerScheme() should reject with: "DAOController__CannotRegisterSchemeWithNullParamsHash"', async function () { const newSchemeAddress = accounts[10]; await expectRevert( controller.registerScheme( newSchemeAddress, - defaultParamsHash, - true, + helpers.constants.NULL_HASH, true, true, - { from: newSchemeAddress } + true ), - "DAOController__SenderNotRegistered" + "DAOController__CannotRegisterSchemeWithNullParamsHash" ); }); @@ -205,21 +204,6 @@ contract("DAOController", function (accounts) { ); }); - it("unregisterScheme() should fail from onlyRegisteredScheme modifyer", async () => { - await controller.registerScheme( - accounts[2], - defaultParamsHash, - true, - true, - true - ); - await controller.unregisterScheme(schemeAddress); - await expectRevert( - controller.unregisterScheme(schemeAddress, { from: schemeAddress }), - "DAOController__SenderNotRegistered" - ); - }); - it("unregisterScheme() should fail from onlyRegisteredScheme modifyer", async () => { await controller.registerScheme( accounts[1], @@ -295,32 +279,6 @@ contract("DAOController", function (accounts) { expectEvent.notEmitted(tx.receipt, "UnregisterScheme"); }); - it("avatarCall() should fail from onlyRegisteredScheme modifyer", async () => { - const newScheme = accounts[2]; - await controller.registerScheme( - newScheme, - defaultParamsHash, - true, - true, - true - ); - - // unregister scheme - await controller.unregisterScheme(schemeAddress); - - await expectRevert( - controller.avatarCall( - helpers.constants.SOME_ADDRESS, - new web3.eth.Contract(DAOAvatar.abi).methods - .executeCall(helpers.constants.SOME_ADDRESS, "0x0", 0) - .encodeABI(), - avatar.address, - 0 - ), - "DAOController__SenderNotRegistered" - ); - }); - it("avatarCall() should fail from onlyAvatarCallScheme modifyer", async () => { await controller.registerScheme( schemeAddress, @@ -479,26 +437,6 @@ contract("DAOController", function (accounts) { expect(await reputation.owner()).to.equal(newOwner); }); - it("isSchemeRegistered() should return if scheme is registered", async () => { - const isRegistered1 = await controller.isSchemeRegistered(schemeAddress); - expect(isRegistered1).to.equal(true); - - // register new scheme to bypass last-scheme unregister check - const newSchemeAddress = accounts[1]; - await controller.registerScheme( - newSchemeAddress, - defaultParamsHash, - true, - true, - true - ); - - await controller.unregisterScheme(schemeAddress); - - const isRegistered2 = await controller.isSchemeRegistered(schemeAddress); - expect(isRegistered2).to.equal(false); - }); - it("getDaoReputation() should return reputationToken address", async () => { const rep = await controller.getDaoReputation(); expect(rep).to.equal(reputation.address); diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index b9b5574c..44926194 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -299,10 +299,6 @@ contract("WalletScheme", function (accounts) { ]); assert.deepEqual(organizationProposal1.value, ["0", "0", "0"]); - assert.equal( - await org.controller.isSchemeRegistered(newWalletScheme.address), - true - ); assert.equal( await org.controller.getSchemeParameters(newWalletScheme.address), defaultParamsHash @@ -316,10 +312,6 @@ contract("WalletScheme", function (accounts) { false ); - assert.equal( - await org.controller.isSchemeRegistered(quickWalletScheme.address), - false - ); assert.equal( await org.controller.getSchemeParameters(quickWalletScheme.address), "0x0000000000000000000000000000000000000000000000000000000000000000" @@ -335,10 +327,6 @@ contract("WalletScheme", function (accounts) { false ); - assert.equal( - await org.controller.isSchemeRegistered(masterWalletScheme.address), - true - ); assert.equal( await org.controller.getSchemeParameters(masterWalletScheme.address), newParamsHash From c79ed50a560011c655e11c1399202b14ba7aa1a2 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 08:57:40 -0300 Subject: [PATCH 06/24] fix(contracts/dao): send stake done in wrong option to dao avatar instead of staker --- contracts/dao/votingMachine/VotingMachine.sol | 35 ++++--- test/dao/votingMachines/VotingMachine.js | 96 ++++++++++++++++--- 2 files changed, 105 insertions(+), 26 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index c4e0e87e..aa22d1e5 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -376,22 +376,29 @@ contract VotingMachine { // If there is staked unclaimed if (staker.amount > 0) { - // If proposal ended and the stake was in the winning option - if ((proposal.state != ProposalState.Expired) && (staker.option == proposal.winningVote)) { - // The reward would be a % (of the staked on the winning option) of all the stakes - reward = - (staker.amount * totalStakesWithoutDaoBounty) / - proposalStakes[proposalId][proposal.winningVote]; - - // If the winning option was yes the reward also include a % (of the staked on the winning option) - // of the minimum dao bounty - if (staker.option == YES) { - uint256 daoBountyReward = (staker.amount * params.daoBounty) / + // If the proposal didnt expired return the staked tokens + if (proposal.state != ProposalState.Expired) { + // If the stake was in the winning option the beneficiary gets the reward + if (staker.option == proposal.winningVote) { + // The reward would be a % (of the staked on the winning option) of all the stakes + reward = + (staker.amount * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; - if (daoBountyReward < stakingToken.allowance(getProposalAvatar(proposalId), address(this))) - stakingToken.transferFrom(getProposalAvatar(proposalId), beneficiary, daoBountyReward); - else emit UnclaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + // If the winning option was yes the reward also include a % (of the staked on the winning option) + // of the minimum dao bounty + if (staker.option == YES) { + uint256 daoBountyReward = (staker.amount * params.daoBounty) / + proposalStakes[proposalId][proposal.winningVote]; + + if (daoBountyReward < stakingToken.allowance(getProposalAvatar(proposalId), address(this))) + stakingToken.transferFrom(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + else emit UnclaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + } + + // If the stake was done on the wrong option and the proposal didnt expired the beneficiary of the reward is the dao avatar and not the staker + } else { + beneficiary = schemes[proposal.schemeId].avatar; } } staker.amount = 0; diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index a7f794ee..c4364cff 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1651,7 +1651,6 @@ contract("VotingMachine", function (accounts) { constants.SOME_HASH ); const testProposalId = await helpers.getValueFromLogs(tx, "proposalId"); - const testProposal = await dxdVotingMachine.proposals(testProposalId); const signerNonce = await dxdVotingMachine.signerNonce(accounts[1]); const stakeHash = await dxdVotingMachine.hashAction( @@ -1746,6 +1745,89 @@ contract("VotingMachine", function (accounts) { ); }); + it("Shouldnt be able to claim a downstake of a boosted executed proposal", async function () { + const tx = await masterAvatarScheme.proposeCalls( + [actionMock.address], + [helpers.testCallFrom(org.avatar.address)], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const testProposalId = await helpers.getValueFromLogs(tx, "proposalId"); + + await dxdVotingMachine.stake( + testProposalId, + constants.NO_OPTION, + web3.utils.toWei("0.1"), + { + from: accounts[2], + } + ); + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("0.3"), + { + from: accounts[1], + } + ); + + await time.increase( + helpers.defaultParameters.preBoostedVotePeriodLimit + 1 + ); + + const voteTx = await dxdVotingMachine.vote( + testProposalId, + constants.YES_OPTION, + 0, + { + from: accounts[2], + gasPrice: constants.GAS_PRICE, + } + ); + + expectEvent(voteTx.receipt, "StateChange", { + proposalId: testProposalId, + proposalState: constants.VOTING_MACHINE_PROPOSAL_STATES.Boosted, + }); + + await time.increase(helpers.defaultParameters.boostedVotePeriodLimit + 1); + + const executeTx = await dxdVotingMachine.execute(testProposalId, { + from: accounts[1], + gasPrice: constants.GAS_PRICE, + }); + + await expectEvent.inTransaction( + executeTx.tx, + dxdVotingMachine.contract, + "StateChange", + { + proposalId: testProposalId, + proposalState: + constants.VOTING_MACHINE_PROPOSAL_STATES.ExecutedInBoost, + } + ); + + const redeemStakeWithNoTx = await dxdVotingMachine.redeem( + testProposalId, + accounts[2] + ); + console.log("redeemStakeWithNoTx", redeemStakeWithNoTx); + + await expectEvent.inTransaction( + redeemStakeWithNoTx.tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: org.avatar.address, + value: web3.utils.toWei("0.1"), + } + ); + }); + it("Stake on multiple proposals in a row and check threshold increase", async function () { const testProposalId1 = await helpers.getValueFromLogs( await masterAvatarScheme.proposeCalls( @@ -1791,17 +1873,7 @@ contract("VotingMachine", function (accounts) { ), "proposalId" ); - const testProposalId5 = await helpers.getValueFromLogs( - await masterAvatarScheme.proposeCalls( - [actionMock.address], - [helpers.testCallFrom(org.avatar.address)], - [0], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ), - "proposalId" - ); + const schemeId = (await dxdVotingMachine.proposals(testProposalId1)) .schemeId; const paramsHash = (await dxdVotingMachine.proposals(testProposalId1)) From 045f7ba4dcb2dfba374e70aea05aef42ef04de9a Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 08:59:31 -0300 Subject: [PATCH 07/24] refactor(contracts/dao): change calculateBoosteChange in VM to external --- contracts/dao/votingMachine/VotingMachine.sol | 2 +- test/dao/votingMachines/VotingMachine.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index aa22d1e5..003f389a 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -473,7 +473,7 @@ contract VotingMachine { * @param proposalId the ID of the proposal * @return toBoost Stake amount needed to boost proposal and move it to preBoost */ - function calculateBoostChange(bytes32 proposalId) public view returns (uint256 toBoost) { + function calculateBoostChange(bytes32 proposalId) external view returns (uint256 toBoost) { Proposal memory proposal = proposals[proposalId]; uint256 thresholdWithPreBoosted = calculateThreshold( parameters[proposal.paramsHash].thresholdConst, diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index c4364cff..ad1c5d24 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1814,7 +1814,6 @@ contract("VotingMachine", function (accounts) { testProposalId, accounts[2] ); - console.log("redeemStakeWithNoTx", redeemStakeWithNoTx); await expectEvent.inTransaction( redeemStakeWithNoTx.tx, From 407e113357c2e469b82342f5e1b3297c2df13da0 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 09:13:37 -0300 Subject: [PATCH 08/24] refactor(contracts/dao): remove unused variable secondsFromTimeOutTillExec in VM --- contracts/dao/votingMachine/VotingMachine.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 003f389a..4bc20aff 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -96,7 +96,6 @@ contract VotingMachine { bytes32 paramsHash; uint256 daoBounty; uint256 totalStakes; // Total number of tokens staked which can be redeemable by stakers. - uint256 secondsFromTimeOutTillExecuteBoosted; uint256[3] times; // times[0] - submittedTime // times[1] - boostedPhaseTime From 5a5442fd3d5bcadd77dad4e015aaf00d9a930be5 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 09:15:07 -0300 Subject: [PATCH 09/24] refactor(contracts/dao): check totalOptions againts fixed NUM_OF_OPTIONS in VM --- contracts/dao/votingMachine/VotingMachine.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 4bc20aff..c5b923c3 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -688,7 +688,7 @@ contract VotingMachine { address proposer, address avatar ) external returns (bytes32 proposalId) { - return _propose(NUM_OF_OPTIONS, paramsHash, proposer, avatar); + return _propose(totalOptions, paramsHash, proposer, avatar); } /** @@ -987,7 +987,7 @@ contract VotingMachine { address proposer, address avatar ) internal returns (bytes32 proposalId) { - if (optionsAmount < NUM_OF_OPTIONS) { + if (optionsAmount != NUM_OF_OPTIONS) { revert VotingMachine__InvalidOptionsAmount(); } // Check parameters existence. From 58ce06c38409ff7df17a8f3f0741908ffca14110 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 10:31:09 -0300 Subject: [PATCH 10/24] refactor(contracts/dao): change state of scheme proposal before execution According to the final audit report is better to do all state changes before doing arbitrary calls, so the state of the proposal in the scheme now it is changed in storage before the calls are done, despite the try and catch used in the voting machine it is possible for that state storage change if executeProposal reverts, this is why after executeProposal we still cann finishProposal, that it will only change the state form submitted to passed or rejected if it wasnt done in the proposal execution. --- contracts/dao/schemes/Scheme.sol | 35 +++++++++++-------- contracts/dao/votingMachine/VotingMachine.sol | 4 +++ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index 86e1af7d..e4919551 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -176,7 +176,8 @@ abstract contract Scheme is VotingMachineCallbacks { } /** - * @dev Execution of proposals, can only be called by the voting machine in which the vote is held. + * @dev Execute the proposal calls for the winning option, + * can only be called by the voting machine in which the vote is held. * @param proposalId The ID of the voting in the voting machine * @param winningOption The winning option in the voting machine * @return success Success of the execution @@ -193,13 +194,18 @@ abstract contract Scheme is VotingMachineCallbacks { } executingProposal = true; - Proposal memory proposal = proposals[proposalId]; + Proposal storage proposal = proposals[proposalId]; if (proposal.state != ProposalState.Submitted) { revert Scheme__ProposalMustBeSubmitted(); } - if (winningOption > 1) { + if (winningOption == 1) { + proposal.state = ProposalState.Rejected; + emit ProposalStateChange(proposalId, uint256(ProposalState.Rejected)); + } else { + proposal.state = ProposalState.Passed; + emit ProposalStateChange(proposalId, uint256(ProposalState.Passed)); uint256 oldRepSupply = getNativeReputationTotalSupply(); permissionRegistry.setERC20Balances(); @@ -253,7 +259,8 @@ abstract contract Scheme is VotingMachineCallbacks { } /** - * @dev Finish a proposal and set the final state in storage + * @dev Finish a proposal and set the final state in storage without any execution. + * The only thing done here is a change in the proposal state in case the proposal was not executed. * @param proposalId The ID of the voting in the voting machine * @param winningOption The winning option in the voting machine * @return success Proposal finish successfully @@ -265,18 +272,18 @@ abstract contract Scheme is VotingMachineCallbacks { returns (bool success) { Proposal storage proposal = proposals[proposalId]; - if (proposal.state != ProposalState.Submitted) { - revert Scheme__ProposalMustBeSubmitted(); - } - - if (winningOption == 1) { - proposal.state = ProposalState.Rejected; - emit ProposalStateChange(proposalId, uint256(ProposalState.Rejected)); + if (proposal.state == ProposalState.Submitted) { + if (winningOption == 1) { + proposal.state = ProposalState.Rejected; + emit ProposalStateChange(proposalId, uint256(ProposalState.Rejected)); + } else { + proposal.state = ProposalState.Passed; + emit ProposalStateChange(proposalId, uint256(ProposalState.Passed)); + } + return true; } else { - proposal.state = ProposalState.Passed; - emit ProposalStateChange(proposalId, uint256(ProposalState.Passed)); + return false; } - return true; } /** diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index c5b923c3..7a1ed761 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -881,6 +881,7 @@ contract VotingMachine { inactiveProposals[getProposalAvatar(proposalId)].add(proposalId); emit ExecuteProposal(proposalId, schemes[proposal.schemeId].avatar, proposal.winningVote, totalReputation); + // Try to execute the proposal for the winning option and catch error if any try ProposalExecuteInterface(proposal.callbacks).executeProposal(proposalId, proposal.winningVote) { emit ProposalExecuteResult(""); } catch Error(string memory errorMessage) { @@ -893,6 +894,9 @@ contract VotingMachine { proposal.executionState = ExecutionState.Failed; emit ProposalExecuteResult(string(errorMessage)); } + + // Set the proposal as executed without executing it, this is done in case the proposal state + // didnt change in the storage on the previous execution ProposalExecuteInterface(proposal.callbacks).finishProposal(proposalId, proposal.winningVote); } if (tmpProposal.state != proposal.state) { From b347ad21e71ce7363ce013bd802f6c3e8eb1f390 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 19 Jan 2023 11:07:55 -0300 Subject: [PATCH 11/24] refactor(contracts/dao): use reveer error message instead fo require in VMCallbacks --- contracts/dao/votingMachine/VotingMachineCallbacks.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachineCallbacks.sol b/contracts/dao/votingMachine/VotingMachineCallbacks.sol index 3f1b9842..cb5676b8 100644 --- a/contracts/dao/votingMachine/VotingMachineCallbacks.sol +++ b/contracts/dao/votingMachine/VotingMachineCallbacks.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.17; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "../DAOController.sol"; import "../DAOReputation.sol"; -import "hardhat/console.sol"; import "./IVotingMachine.sol"; contract VotingMachineCallbacks { @@ -12,8 +11,10 @@ contract VotingMachineCallbacks { DAOController public controller; + error VotingMachineCallbacks__OnlyVotingMachine(); + modifier onlyVotingMachine() { - require(msg.sender == address(votingMachine), "VotingMachineCallbacks: only VotingMachine"); + if (msg.sender != address(votingMachine)) revert VotingMachineCallbacks__OnlyVotingMachine(); _; } From a7c6a2c617daaf89f24be68f0b509d5733000893 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Mon, 23 Jan 2023 09:48:41 -0300 Subject: [PATCH 12/24] refactor(contracts/dao): make Scheme initializable and deploy it in tests using Create2Deployer --- contracts/dao/schemes/Scheme.sol | 12 ++---- contracts/utils/Create2Deployer.sol | 28 ++++++++++++- test/dao/schemes/WalletScheme.js | 65 ++++++++++++++++++----------- test/helpers/index.js | 26 ++++++++++++ 4 files changed, 95 insertions(+), 36 deletions(-) diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index e4919551..25e621fb 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.17; import "@openzeppelin/contracts/utils/Address.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import "../../utils/PermissionRegistry.sol"; import "../DAOReputation.sol"; import "../DAOAvatar.sol"; @@ -25,7 +26,7 @@ import "../votingMachine/VotingMachineCallbacks.sol"; * Once the governance process ends on the voting machine the voting machine can execute the proposal winning option. * If the wining option cant be executed successfully, it can be finished without execution once the maxTimesForExecution time passes. */ -abstract contract Scheme is VotingMachineCallbacks { +abstract contract Scheme is Initializable, VotingMachineCallbacks { using Address for address; enum ProposalState { @@ -59,9 +60,6 @@ abstract contract Scheme is VotingMachineCallbacks { event ProposalStateChange(bytes32 indexed proposalId, uint256 indexed state); - /// @notice Emitted when its initialized twice - error Scheme__CannotInitTwice(); - /// @notice Emitted if avatar address is zero error Scheme__AvatarAddressCannotBeZero(); @@ -105,11 +103,7 @@ abstract contract Scheme is VotingMachineCallbacks { address permissionRegistryAddress, string calldata _schemeName, uint256 _maxRepPercentageChange - ) external { - if (address(avatar) != address(0)) { - revert Scheme__CannotInitTwice(); - } - + ) external initializer { if (avatarAddress == address(0)) { revert Scheme__AvatarAddressCannotBeZero(); } diff --git a/contracts/utils/Create2Deployer.sol b/contracts/utils/Create2Deployer.sol index fc0a411d..b9fee89f 100644 --- a/contracts/utils/Create2Deployer.sol +++ b/contracts/utils/Create2Deployer.sol @@ -2,7 +2,9 @@ pragma solidity ^0.8.17; contract Create2Deployer { - event Deployed(address addr, uint256 salt); + error Create2Deployer__InitializedFailed(); + + event Deployed(address addr, bytes32 bytecodeHash, uint256 salt); function deploy(bytes memory code, uint256 salt) public { address addr; @@ -13,6 +15,28 @@ contract Create2Deployer { } } - emit Deployed(addr, salt); + emit Deployed(addr, keccak256(abi.encodePacked(code)), salt); + } + + function deployAndInitialize( + bytes memory code, + uint256 salt, + bytes memory initializeCallData + ) public { + address addr; + assembly { + addr := create2(0, add(code, 0x20), mload(code), salt) + if iszero(extcodesize(addr)) { + revert(0, 0) + } + } + + (bool initializeSuccess, ) = addr.call{value: 0}(initializeCallData); + + if (!initializeSuccess) { + revert Create2Deployer__InitializedFailed(); + } + + emit Deployed(addr, keccak256(abi.encodePacked(code)), salt); } } diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 44926194..6524cdf8 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -7,6 +7,7 @@ const { expectEvent, } = require("@openzeppelin/test-helpers"); +const Create2Deployer = artifacts.require("./Create2Deployer.sol"); const WalletScheme = artifacts.require("./WalletScheme.sol"); const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); @@ -45,34 +46,48 @@ contract("WalletScheme", function (accounts) { permissionRegistry = await PermissionRegistry.new(accounts[0], 30); await permissionRegistry.initialize(); - registrarScheme = await WalletScheme.new(); - await registrarScheme.initialize( - org.avatar.address, - org.votingMachine.address, - org.controller.address, - permissionRegistry.address, - "Wallet Scheme Registrar", - 0 + const create2Deployer = await Create2Deployer.new(); + + registrarScheme = await helpers.deployContractWithCreate2( + create2Deployer, + WalletScheme, + web3.utils.keccak256("registrarScheme1"), + [ + org.avatar.address, + org.votingMachine.address, + org.controller.address, + permissionRegistry.address, + "Wallet Scheme Registrar", + 0, + ] ); - masterWalletScheme = await WalletScheme.new(); - await masterWalletScheme.initialize( - org.avatar.address, - org.votingMachine.address, - org.controller.address, - permissionRegistry.address, - "Master Wallet", - 5 + masterWalletScheme = await helpers.deployContractWithCreate2( + create2Deployer, + WalletScheme, + web3.utils.keccak256("masterWalletScheme1"), + [ + org.avatar.address, + org.votingMachine.address, + org.controller.address, + permissionRegistry.address, + "Master Wallet", + 5, + ] ); - quickWalletScheme = await WalletScheme.new(); - await quickWalletScheme.initialize( - org.avatar.address, - org.votingMachine.address, - org.controller.address, - permissionRegistry.address, - "Quick Wallet", - 1 + quickWalletScheme = await helpers.deployContractWithCreate2( + create2Deployer, + WalletScheme, + web3.utils.keccak256("quickWalletScheme1"), + [ + org.avatar.address, + org.votingMachine.address, + org.controller.address, + permissionRegistry.address, + "Quick Wallet", + 1, + ] ); await permissionRegistry.setETHPermission( @@ -1331,7 +1346,7 @@ contract("WalletScheme", function (accounts) { "Master Wallet", 5 ), - "Scheme__CannotInitTwice()" + "Initializable: contract is already initialized" ); }); diff --git a/test/helpers/index.js b/test/helpers/index.js index 0ea01fbb..4ddb34dc 100644 --- a/test/helpers/index.js +++ b/test/helpers/index.js @@ -57,6 +57,32 @@ export function getValueFromLogs(tx, arg, eventName, index = 0) { return result; } +export const deployContractWithCreate2 = async function ( + create2Contract, + contractToDeploy, + salt = constants.SOME_HASH, + initilizerArgs = [] +) { + const newContractAddress = create2Address( + create2Contract.address, + contractToDeploy.bytecode, + salt + ); + if (initilizerArgs.length > 0) { + await create2Contract.deployAndInitialize( + contractToDeploy.bytecode, + salt, + web3.eth.abi.encodeFunctionCall( + contractToDeploy.abi.find(x => x.name === "initialize"), + initilizerArgs + ) + ); + } else { + await create2Contract.deploy(contractToDeploy.bytecode, salt); + } + return await contractToDeploy.at(newContractAddress); +}; + export const deployDao = async function (deployConfig) { const reputation = await DAOReputation.new(); await reputation.initialize("DXDaoReputation", "DXRep"); From 2c6d09905492c70b4ce5903581da115f77f6605c Mon Sep 17 00:00:00 2001 From: AugustoL Date: Mon, 23 Jan 2023 17:34:54 -0300 Subject: [PATCH 13/24] refactor(contracts/dao): remove multiplyRealMath from VotingMachine --- contracts/dao/votingMachine/VotingMachine.sol | 7 ------- test/dao/votingMachines/VotingMachine.js | 10 +++++----- test/helpers/index.js | 9 +++++++++ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 7a1ed761..080aa070 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -1275,11 +1275,4 @@ contract VotingMachine { function getInactiveProposalsCount(address avatar) public view returns (uint256 inactiveProposalsCount) { return inactiveProposals[avatar].length(); } - - /** - * @dev Helper function used in test to execute a real math lib multiplication - */ - function multiplyRealMath(uint256 a, uint256 b) public pure returns (uint256) { - return a.mul(b); - } } diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index ad1c5d24..a64a3d39 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1880,11 +1880,11 @@ contract("VotingMachine", function (accounts) { const schemeParameters = await dxdVotingMachine.parameters(paramsHash); const threshold0BoostedProposal = await dxdVotingMachine.getSchemeThreshold(paramsHash, schemeId); - const stakesToBoostFirstProposal = - await dxdVotingMachine.multiplyRealMath( - threshold0BoostedProposal, - schemeParameters.daoBounty - ); + + const stakesToBoostFirstProposal = helpers.multiplyRealMath( + threshold0BoostedProposal, + schemeParameters.daoBounty + ); // Stakes just what it needs to get to the boost threshold await dxdVotingMachine.stake( diff --git a/test/helpers/index.js b/test/helpers/index.js index 4ddb34dc..5e61b2e2 100644 --- a/test/helpers/index.js +++ b/test/helpers/index.js @@ -279,4 +279,13 @@ export function customErrorMessageExistInRawLogs( ); } +export function multiplyRealMath(realA, realB) { + const BN = web3.utils.BN; + let res = new BN(realA).mul(new BN(realB)); + if (!res.div(new BN(realA)).eq(new BN(realB))) { + throw new Error("RealMath mul overflow"); + } + return res.ushrn(40); +} + export { constants }; From 8d02eccc1f02d809490897898092563bc8694a44 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Mon, 13 Feb 2023 08:37:43 -0300 Subject: [PATCH 14/24] fix(contracts/dao): dont return stake on loosing option and check for staking token transfer success --- contracts/dao/votingMachine/VotingMachine.sol | 77 ++++++++++--------- test/dao/votingMachines/VotingMachine.js | 12 ++- 2 files changed, 48 insertions(+), 41 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 6562acae..d580f923 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -171,6 +171,7 @@ contract VotingMachine { /// @notice Emited when proposal is not in ExecutedInQueue, ExecutedInBoost or Expired status error VotingMachine__WrongProposalStateToRedeem(); error VotingMachine__TransferFailed(address to, uint256 amount); + error VotingMachine__TransferFromFailed(address to, uint256 amount); /// @notice Emited when proposal is not in ExecutedInQueue or ExecutedInBoost status error VotingMachine__WrongProposalStateToRedeemDaoBounty(); @@ -364,54 +365,56 @@ contract VotingMachine { revert VotingMachine__WrongProposalStateToRedeem(); } - Parameters memory params = parameters[proposal.paramsHash]; Staker storage staker = proposalStakers[proposalId][beneficiary]; - // Default reward is the stakes amount - reward = staker.amount; uint256 totalStakesWithoutDaoBounty = proposalStakes[proposalId][NO] + proposalStakes[proposalId][YES] - proposal.daoBounty; - // If there is staked unclaimed - if (staker.amount > 0) { - // If the proposal didnt expired return the staked tokens - if (proposal.state != ProposalState.Expired) { - // If the stake was in the winning option the beneficiary gets the reward - if (staker.option == proposal.winningVote) { - // The reward would be a % (of the staked on the winning option) of all the stakes - reward = - (staker.amount * totalStakesWithoutDaoBounty) / - proposalStakes[proposalId][proposal.winningVote]; - - // If the winning option was yes the reward also include a % (of the staked on the winning option) - // of the minimum dao bounty - if (staker.option == YES) { - uint256 daoBountyReward = (staker.amount * params.daoBounty) / - proposalStakes[proposalId][proposal.winningVote]; - - if (daoBountyReward < stakingToken.allowance(getProposalAvatar(proposalId), address(this))) - stakingToken.transferFrom(getProposalAvatar(proposalId), beneficiary, daoBountyReward); - else emit UnclaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); - } + // If there is stake unclaimed + // If the proposal didnt expired return the staked tokens + // If the stake was in the winning option the beneficiary gets the reward + if ( + (staker.amount > 0) && (proposal.state != ProposalState.Expired) && (staker.option == proposal.winningVote) + ) { + // The reward would be a % (of the staked on the winning option) of all the stakes + reward = (staker.amount * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; - // If the stake was done on the wrong option and the proposal didnt expired the beneficiary of the reward is the dao avatar and not the staker - } else { - beneficiary = schemes[proposal.schemeId].avatar; + bool transferSuccess; + + if (reward > 0) { + proposal.totalStakes = proposal.totalStakes - reward; + schemes[proposal.schemeId].stakingTokenBalance = + schemes[proposal.schemeId].stakingTokenBalance - + reward; + + transferSuccess = stakingToken.transfer(beneficiary, reward); + if (!transferSuccess) { + revert VotingMachine__TransferFailed(beneficiary, reward); } + emit Redeem(proposalId, schemes[proposal.schemeId].avatar, beneficiary, reward); } - staker.amount = 0; - } - if (reward != 0) { - proposal.totalStakes = proposal.totalStakes - reward; - schemes[proposal.schemeId].stakingTokenBalance = schemes[proposal.schemeId].stakingTokenBalance - reward; - - bool transferSuccess = stakingToken.transfer(beneficiary, reward); - if (!transferSuccess) { - revert VotingMachine__TransferFailed(beneficiary, reward); + // If the winning option was yes the reward also include a % (of the staked on the winning option) + // of the minimum dao bounty + if (staker.option == YES) { + uint256 daoBountyReward = (staker.amount * parameters[proposal.paramsHash].daoBounty) / + proposalStakes[proposalId][proposal.winningVote]; + + transferSuccess = stakingToken.transferFrom( + getProposalAvatar(proposalId), + beneficiary, + daoBountyReward + ); + if (!transferSuccess) { + revert VotingMachine__TransferFromFailed(beneficiary, daoBountyReward); + } else { + emit UnclaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + } } - emit Redeem(proposalId, schemes[proposal.schemeId].avatar, beneficiary, reward); + + // The staker amount is marked as 0 to make sure the staker can't redeem twice + staker.amount = 0; } } diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index a64a3d39..d9d67560 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1261,12 +1261,16 @@ contract("VotingMachine", function (accounts) { web3.utils.toWei("0.41") ); - await dxdVotingMachine.redeem(fakeProposalId, accounts[9]); + // The daoBounty redeems fails cause it cames form the fakeOrg avatar and it has no tokens, so the redeem cant be done + await expectRevert( + dxdVotingMachine.redeem(fakeProposalId, accounts[9]), + "ERC20: transfer amount exceeds balance" + ); - // If the attack succedded this should be 0 + // If the attack succeeded this should be 0 assert.equal( await stakingToken.balanceOf(dxdVotingMachine.address), - web3.utils.toWei("0.2") + web3.utils.toWei("0.41") ); // attack ends @@ -1815,7 +1819,7 @@ contract("VotingMachine", function (accounts) { accounts[2] ); - await expectEvent.inTransaction( + await expectEvent.notEmitted.inTransaction( redeemStakeWithNoTx.tx, stakingToken.contract, "Transfer", From c4f440949f4fe134d9ef75b874f65070b5c8b7fe Mon Sep 17 00:00:00 2001 From: AugustoL Date: Fri, 17 Feb 2023 09:37:34 -0300 Subject: [PATCH 15/24] fix(contracts/dao): fix wrong name of UnclaimedDaoBounty event in VM to claimed --- contracts/dao/votingMachine/VotingMachine.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index d580f923..bb4b31a2 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -145,7 +145,7 @@ contract VotingMachine { event Redeem(bytes32 indexed proposalId, address indexed avatar, address indexed beneficiary, uint256 amount); - event UnclaimedDaoBounty(address indexed avatar, address beneficiary, uint256 amount); + event ClaimedDaoBounty(address indexed avatar, address beneficiary, uint256 amount); event ActionSigned( bytes32 proposalId, @@ -409,7 +409,7 @@ contract VotingMachine { if (!transferSuccess) { revert VotingMachine__TransferFromFailed(beneficiary, daoBountyReward); } else { - emit UnclaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + emit ClaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); } } From 0067ddc0dbcc499501513c34fac67ecbcdda0c03 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Fri, 17 Feb 2023 09:39:55 -0300 Subject: [PATCH 16/24] fix(contracts/dao): set staker amount to 0 before doing the transfer in VM stake redeem --- contracts/dao/votingMachine/VotingMachine.sol | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index bb4b31a2..5f753650 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -371,6 +371,11 @@ contract VotingMachine { proposalStakes[proposalId][YES] - proposal.daoBounty; + uint256 staked = staker.amount; + + // The staker amount is marked as 0 to make sure the staker can't redeem twice + staker.amount = 0; + // If there is stake unclaimed // If the proposal didnt expired return the staked tokens // If the stake was in the winning option the beneficiary gets the reward @@ -378,7 +383,7 @@ contract VotingMachine { (staker.amount > 0) && (proposal.state != ProposalState.Expired) && (staker.option == proposal.winningVote) ) { // The reward would be a % (of the staked on the winning option) of all the stakes - reward = (staker.amount * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; + reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; bool transferSuccess; @@ -398,7 +403,7 @@ contract VotingMachine { // If the winning option was yes the reward also include a % (of the staked on the winning option) // of the minimum dao bounty if (staker.option == YES) { - uint256 daoBountyReward = (staker.amount * parameters[proposal.paramsHash].daoBounty) / + uint256 daoBountyReward = (staked * parameters[proposal.paramsHash].daoBounty) / proposalStakes[proposalId][proposal.winningVote]; transferSuccess = stakingToken.transferFrom( @@ -412,9 +417,6 @@ contract VotingMachine { emit ClaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); } } - - // The staker amount is marked as 0 to make sure the staker can't redeem twice - staker.amount = 0; } } From 8d922218d3a0ad2cdad4d99151ffd2c0f0b71744 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Fri, 17 Feb 2023 09:40:37 -0300 Subject: [PATCH 17/24] fix(contracts/dao): fix wrong condition check in VM redeem --- contracts/dao/votingMachine/VotingMachine.sol | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 5f753650..300bafe2 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -377,11 +377,8 @@ contract VotingMachine { staker.amount = 0; // If there is stake unclaimed - // If the proposal didnt expired return the staked tokens // If the stake was in the winning option the beneficiary gets the reward - if ( - (staker.amount > 0) && (proposal.state != ProposalState.Expired) && (staker.option == proposal.winningVote) - ) { + if ((staked > 0) && (staker.option == proposal.winningVote)) { // The reward would be a % (of the staked on the winning option) of all the stakes reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; From 70f141456b465b567dc9fb3e4072d7f09f841b9a Mon Sep 17 00:00:00 2001 From: AugustoL Date: Tue, 21 Feb 2023 11:14:39 -0300 Subject: [PATCH 18/24] fix(contracts/dao): fix VM redeem to allow dao avatar to claim stakes on winning NO option --- contracts/dao/votingMachine/VotingMachine.sol | 50 ++++-- test/dao/votingMachines/VotingMachine.js | 166 +++++++++++++++++- 2 files changed, 194 insertions(+), 22 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 300bafe2..2a20e532 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -170,6 +170,7 @@ contract VotingMachine { /// @notice Emited when proposal is not in ExecutedInQueue, ExecutedInBoost or Expired status error VotingMachine__WrongProposalStateToRedeem(); + error VotingMachine__NoAmountToRedeem(); error VotingMachine__TransferFailed(address to, uint256 amount); error VotingMachine__TransferFromFailed(address to, uint256 amount); @@ -365,25 +366,36 @@ contract VotingMachine { revert VotingMachine__WrongProposalStateToRedeem(); } + // Check that there are tokens to be redeemed Staker storage staker = proposalStakers[proposalId][beneficiary]; + uint256 staked = staker.amount; + if (staked == 0) { + revert VotingMachine__NoAmountToRedeem(); + } + + // The staker amount is marked as 0 to make sure the staker can't redeem twice + staker.amount = 0; uint256 totalStakesWithoutDaoBounty = proposalStakes[proposalId][NO] + proposalStakes[proposalId][YES] - proposal.daoBounty; - uint256 staked = staker.amount; + bool transferSuccess; + address proposalAvatar = getProposalAvatar(proposalId); - // The staker amount is marked as 0 to make sure the staker can't redeem twice - staker.amount = 0; + // If the proposal expires the staked amount is sent back to the staker + if (proposal.state == ProposalState.Expired) { + transferSuccess = stakingToken.transfer(beneficiary, staked); + if (!transferSuccess) { + revert VotingMachine__TransferFailed(beneficiary, staked); + } + emit Redeem(proposalId, schemes[proposal.schemeId].avatar, beneficiary, staked); - // If there is stake unclaimed - // If the stake was in the winning option the beneficiary gets the reward - if ((staked > 0) && (staker.option == proposal.winningVote)) { + // If the proposal was executed and the stake was in the winning option the beneficiary gets the reward + } else if (staker.option == proposal.winningVote) { // The reward would be a % (of the staked on the winning option) of all the stakes reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; - bool transferSuccess; - if (reward > 0) { proposal.totalStakes = proposal.totalStakes - reward; schemes[proposal.schemeId].stakingTokenBalance = @@ -401,19 +413,27 @@ contract VotingMachine { // of the minimum dao bounty if (staker.option == YES) { uint256 daoBountyReward = (staked * parameters[proposal.paramsHash].daoBounty) / - proposalStakes[proposalId][proposal.winningVote]; + proposalStakes[proposalId][YES]; - transferSuccess = stakingToken.transferFrom( - getProposalAvatar(proposalId), - beneficiary, - daoBountyReward - ); + transferSuccess = stakingToken.transferFrom(proposalAvatar, beneficiary, daoBountyReward); if (!transferSuccess) { revert VotingMachine__TransferFromFailed(beneficiary, daoBountyReward); } else { - emit ClaimedDaoBounty(getProposalAvatar(proposalId), beneficiary, daoBountyReward); + emit ClaimedDaoBounty(proposalAvatar, beneficiary, daoBountyReward); } } + + // If the staker staked on YES, and NO won, the stake is lost and is sent to the avatar + } else if (staker.option == YES && proposal.winningVote == NO) { + uint256 daoBountyReward = (staked * parameters[proposal.paramsHash].daoBounty) / + proposalStakes[proposalId][NO]; + + transferSuccess = stakingToken.transfer(proposalAvatar, daoBountyReward); + if (!transferSuccess) { + revert VotingMachine__TransferFromFailed(proposalAvatar, daoBountyReward); + } else { + emit ClaimedDaoBounty(proposalAvatar, proposalAvatar, daoBountyReward); + } } } diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index 1a3df486..b8f1e4cc 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1319,8 +1319,6 @@ contract("VotingMachine", function (accounts) { ); assert.equal(schemeProposal.to[0], actionMock.address); assert.equal(schemeProposal.value[0], 0); - - await dxdVotingMachine.redeem(testProposalId, accounts[9]); }); it("boosted proposal should fail with not enough votes", async function () { @@ -1814,21 +1812,175 @@ contract("VotingMachine", function (accounts) { } ); - const redeemStakeWithNoTx = await dxdVotingMachine.redeem( + const balanceBeforeRedeem = await stakingToken.balanceOf(accounts[2]); + await dxdVotingMachine.redeem(testProposalId, accounts[2]); + // Nothing redeemed + assert(balanceBeforeRedeem.eq(await stakingToken.balanceOf(accounts[2]))); + }); + + it("Should be able to claim a all stakes of an expired proposal", async function () { + const tx = await masterAvatarScheme.proposeCalls( + [actionMock.address], + [helpers.testCallFrom(org.avatar.address)], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const testProposalId = await helpers.getValueFromLogs(tx, "proposalId"); + + await dxdVotingMachine.stake( testProposalId, - accounts[2] + constants.YES_OPTION, + web3.utils.toWei("0.1"), + { + from: accounts[2], + } + ); + await dxdVotingMachine.stake( + testProposalId, + constants.NO_OPTION, + web3.utils.toWei("0.3"), + { + from: accounts[1], + } + ); + + await time.increase(helpers.defaultParameters.queuedVotePeriodLimit + 1); + + const executeTx = await dxdVotingMachine.execute(testProposalId, { + from: accounts[1], + gasPrice: constants.GAS_PRICE, + }); + + await expectEvent.inTransaction( + executeTx.tx, + dxdVotingMachine.contract, + "StateChange", + { + proposalId: testProposalId, + proposalState: constants.VOTING_MACHINE_PROPOSAL_STATES.Expired, + } ); - await expectEvent.notEmitted.inTransaction( - redeemStakeWithNoTx.tx, + await expectEvent.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, accounts[2]) + ).tx, stakingToken.contract, "Transfer", { from: dxdVotingMachine.address, - to: org.avatar.address, + to: accounts[2], value: web3.utils.toWei("0.1"), } ); + + await expectEvent.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, accounts[1]) + ).tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: accounts[1], + value: web3.utils.toWei("0.3"), + } + ); + }); + + it("Stake on YES is lost and sent to avatar when NO wins", async function () { + const tx = await masterAvatarScheme.proposeCalls( + [actionMock.address], + [helpers.testCallFrom(org.avatar.address)], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const testProposalId = await helpers.getValueFromLogs(tx, "proposalId"); + + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("0.1"), + { + from: accounts[1], + } + ); + await dxdVotingMachine.stake( + testProposalId, + constants.NO_OPTION, + web3.utils.toWei("0.2"), + { + from: accounts[2], + } + ); + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("1"), + { + from: accounts[1], + } + ); + const finalVoteTx = await dxdVotingMachine.vote( + testProposalId, + constants.NO_OPTION, + 0, + { + from: accounts[3], + } + ); + + await expectEvent.inTransaction( + finalVoteTx.tx, + dxdVotingMachine.contract, + "StateChange", + { + proposalId: testProposalId, + proposalState: + constants.VOTING_MACHINE_PROPOSAL_STATES.ExecutedInQueue, + } + ); + + const daoBounty = new BN(helpers.defaultParameters.daoBounty); + const stakedOnYes = new BN(web3.utils.toWei("1.1")); + const stakedOnNo = new BN(web3.utils.toWei("0.2")); + const totalStakesWithoutDaoBounty = stakedOnYes.add(stakedOnNo); + const daoBountyRewardToAvatar = stakedOnYes + .mul(daoBounty) + .div(stakedOnNo.add(daoBounty)); + const redeemForStakedOnNo = stakedOnNo + .mul(totalStakesWithoutDaoBounty) + .div(stakedOnNo.add(daoBounty)); + + await expectEvent.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, accounts[1]) + ).tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: org.avatar.address, + value: daoBountyRewardToAvatar, + } + ); + + await expectEvent.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, accounts[2]) + ).tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: accounts[2], + value: redeemForStakedOnNo, + } + ); }); it("Stake on multiple proposals in a row and check threshold increase", async function () { From 897223b9d459c5576b006cb9873754adc2fab0a5 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 22 Feb 2023 10:41:33 -0300 Subject: [PATCH 19/24] fix(contracts/dao): add daoRedeemedWinnings and keep track of stakingTokenBalance on redeems --- contracts/dao/votingMachine/VotingMachine.sol | 40 +++++++++------ test/dao/votingMachines/VotingMachine.js | 50 ++++++++++++++++--- 2 files changed, 69 insertions(+), 21 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 61a20bec..2295a891 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -96,6 +96,7 @@ contract VotingMachine { bytes32 paramsHash; uint256 daoBounty; uint256 totalStakes; // Total number of tokens staked which can be redeemable by stakers. + bool daoRedeemedWinnings; // True if the DAO has claimed the bounty for this proposal. uint256[3] times; // times[0] - submittedTime // times[1] - boostedPhaseTime @@ -366,10 +367,12 @@ contract VotingMachine { revert VotingMachine__WrongProposalStateToRedeem(); } + address proposalAvatar = getProposalAvatar(proposalId); + // Check that there are tokens to be redeemed Staker storage staker = proposalStakers[proposalId][beneficiary]; uint256 staked = staker.amount; - if (staked == 0) { + if (staked == 0 && beneficiary != proposalAvatar) { revert VotingMachine__NoAmountToRedeem(); } @@ -381,11 +384,30 @@ contract VotingMachine { proposal.daoBounty; bool transferSuccess; - address proposalAvatar = getProposalAvatar(proposalId); + // If NO won and there is staed tokens on YES, the dao avatar gets a % or the rewards + if (beneficiary == proposalAvatar && proposal.winningVote == NO && !proposal.daoRedeemedWinnings) { + uint256 daoBountyReward = (proposalStakes[proposalId][YES] * parameters[proposal.paramsHash].daoBounty) / + proposalStakes[proposalId][NO]; + + schemes[proposal.schemeId].stakingTokenBalance = + schemes[proposal.schemeId].stakingTokenBalance - + daoBountyReward; + + transferSuccess = stakingToken.transfer(proposalAvatar, daoBountyReward); + if (!transferSuccess) { + revert VotingMachine__TransferFromFailed(proposalAvatar, daoBountyReward); + } else { + emit ClaimedDaoBounty(proposalAvatar, proposalAvatar, daoBountyReward); + } + proposal.daoRedeemedWinnings = true; + } // If the proposal expires the staked amount is sent back to the staker - if (proposal.state == ProposalState.Expired) { + else if (proposal.state == ProposalState.Expired) { transferSuccess = stakingToken.transfer(beneficiary, staked); + + schemes[proposal.schemeId].stakingTokenBalance = schemes[proposal.schemeId].stakingTokenBalance - staked; + if (!transferSuccess) { revert VotingMachine__TransferFailed(beneficiary, staked); } @@ -422,18 +444,6 @@ contract VotingMachine { emit ClaimedDaoBounty(proposalAvatar, beneficiary, daoBountyReward); } } - - // If the staker staked on YES, and NO won, the stake is lost and is sent to the avatar - } else if (staker.option == YES && proposal.winningVote == NO) { - uint256 daoBountyReward = (staked * parameters[proposal.paramsHash].daoBounty) / - proposalStakes[proposalId][NO]; - - transferSuccess = stakingToken.transfer(proposalAvatar, daoBountyReward); - if (!transferSuccess) { - revert VotingMachine__TransferFromFailed(proposalAvatar, daoBountyReward); - } else { - emit ClaimedDaoBounty(proposalAvatar, proposalAvatar, daoBountyReward); - } } } diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index b8f1e4cc..077e17f9 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1917,6 +1917,14 @@ contract("VotingMachine", function (accounts) { from: accounts[2], } ); + await dxdVotingMachine.stake( + testProposalId, + constants.NO_OPTION, + web3.utils.toWei("0.2"), + { + from: accounts[3], + } + ); await dxdVotingMachine.stake( testProposalId, constants.YES_OPTION, @@ -1947,18 +1955,23 @@ contract("VotingMachine", function (accounts) { const daoBounty = new BN(helpers.defaultParameters.daoBounty); const stakedOnYes = new BN(web3.utils.toWei("1.1")); - const stakedOnNo = new BN(web3.utils.toWei("0.2")); - const totalStakesWithoutDaoBounty = stakedOnYes.add(stakedOnNo); + const totalStakedNo = new BN(web3.utils.toWei("0.4")); + const totalStakesWithoutDaoBounty = stakedOnYes.add(totalStakedNo); const daoBountyRewardToAvatar = stakedOnYes .mul(daoBounty) - .div(stakedOnNo.add(daoBounty)); - const redeemForStakedOnNo = stakedOnNo + .div(totalStakedNo.add(daoBounty)); + const redeemForStakedOnNo = new BN(web3.utils.toWei("0.2")) .mul(totalStakesWithoutDaoBounty) - .div(stakedOnNo.add(daoBounty)); + .div(totalStakedNo.add(daoBounty)); + + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + false + ); await expectEvent.inTransaction( ( - await dxdVotingMachine.redeem(testProposalId, accounts[1]) + await dxdVotingMachine.redeem(testProposalId, org.avatar.address) ).tx, stakingToken.contract, "Transfer", @@ -1969,6 +1982,19 @@ contract("VotingMachine", function (accounts) { } ); + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + true + ); + + await expectEvent.notEmitted.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, org.avatar.address) + ).tx, + stakingToken.contract, + "Transfer" + ); + await expectEvent.inTransaction( ( await dxdVotingMachine.redeem(testProposalId, accounts[2]) @@ -1981,6 +2007,18 @@ contract("VotingMachine", function (accounts) { value: redeemForStakedOnNo, } ); + await expectEvent.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, accounts[3]) + ).tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: accounts[3], + value: redeemForStakedOnNo, + } + ); }); it("Stake on multiple proposals in a row and check threshold increase", async function () { From 72f02af270b88b04c2cf6ec2377445a34efe8bf8 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 22 Feb 2023 11:26:50 -0300 Subject: [PATCH 20/24] refactor(contracts/dao): remove unnecesary totalStakes in proposals stuct in VM --- contracts/dao/votingMachine/VotingMachine.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 2295a891..9160db37 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -419,7 +419,6 @@ contract VotingMachine { reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][proposal.winningVote]; if (reward > 0) { - proposal.totalStakes = proposal.totalStakes - reward; schemes[proposal.schemeId].stakingTokenBalance = schemes[proposal.schemeId].stakingTokenBalance - reward; @@ -990,7 +989,6 @@ contract VotingMachine { revert VotingMachine__TransferFromStakerFailed(); } schemes[proposal.schemeId].stakingTokenBalance += amount; - proposal.totalStakes = proposal.totalStakes + amount; //update totalRedeemableStakes proposalStake.amount = proposalStake.amount + amount; proposalStake.option = option; @@ -1000,7 +998,10 @@ contract VotingMachine { revert VotingMachine__StakingAmountIsTooHight(); } - if (proposal.totalStakes > uint256(0x100000000000000000000000000000000)) { + if ( + proposalStakes[proposalId][YES] + proposalStakes[proposalId][NO] > + uint256(0x100000000000000000000000000000000) + ) { revert VotingMachine__TotalStakesIsToHight(); } From 9b5c4c8d01da13814907ffe934559a438dc1c253 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 23 Feb 2023 11:42:52 -0300 Subject: [PATCH 21/24] fix(contracts/dao): removed unused totalStakes in proposal struct in VM --- contracts/dao/votingMachine/VotingMachine.sol | 1 - test/dao/votingMachines/VotingMachine.js | 10 ---------- 2 files changed, 11 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 9160db37..87fa8c77 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -95,7 +95,6 @@ contract VotingMachine { uint256 currentBoostedVotePeriodLimit; bytes32 paramsHash; uint256 daoBounty; - uint256 totalStakes; // Total number of tokens staked which can be redeemable by stakers. bool daoRedeemedWinnings; // True if the DAO has claimed the bounty for this proposal. uint256[3] times; // times[0] - submittedTime diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index 077e17f9..b200b027 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -1413,11 +1413,6 @@ contract("VotingMachine", function (accounts) { } ); - const totalStaked = (await dxdVotingMachine.proposals(proposalId)) - .totalStakes; - - assert(totalStaked.eq(stakesToBoost)); - // check preBoosted expectEvent(upStake.receipt, "StateChange", { proposalId: proposalId, @@ -1483,11 +1478,6 @@ contract("VotingMachine", function (accounts) { } ); - const totalStaked = (await dxdVotingMachine.proposals(proposalId)) - .totalStakes; - - assert(totalStaked.eq(stakesToBoost)); - // check preBoosted expectEvent(upStake.receipt, "StateChange", { proposalId: proposalId, From b0f55c8f20d9ce95e9f7350ee26cd4b55e110a09 Mon Sep 17 00:00:00 2001 From: AugustoL Date: Thu, 23 Feb 2023 12:01:55 -0300 Subject: [PATCH 22/24] refactor(contracts/dao): check that proposal expired before daoRedeemWinnings in VM redeem --- contracts/dao/votingMachine/VotingMachine.sol | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 87fa8c77..f5939b23 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -384,8 +384,19 @@ contract VotingMachine { bool transferSuccess; - // If NO won and there is staed tokens on YES, the dao avatar gets a % or the rewards - if (beneficiary == proposalAvatar && proposal.winningVote == NO && !proposal.daoRedeemedWinnings) { + // If the proposal expires the staked amount is sent back to the staker + if (proposal.state == ProposalState.Expired) { + schemes[proposal.schemeId].stakingTokenBalance = schemes[proposal.schemeId].stakingTokenBalance - staked; + + transferSuccess = stakingToken.transfer(beneficiary, staked); + + if (!transferSuccess) { + revert VotingMachine__TransferFailed(beneficiary, staked); + } + emit Redeem(proposalId, proposalAvatar, beneficiary, staked); + + // If NO won and there is staed tokens on YES, the dao avatar gets a % or the rewards + } else if (beneficiary == proposalAvatar && proposal.winningVote == NO && !proposal.daoRedeemedWinnings) { uint256 daoBountyReward = (proposalStakes[proposalId][YES] * parameters[proposal.paramsHash].daoBounty) / proposalStakes[proposalId][NO]; @@ -393,24 +404,14 @@ contract VotingMachine { schemes[proposal.schemeId].stakingTokenBalance - daoBountyReward; + proposal.daoRedeemedWinnings = true; + transferSuccess = stakingToken.transfer(proposalAvatar, daoBountyReward); if (!transferSuccess) { revert VotingMachine__TransferFromFailed(proposalAvatar, daoBountyReward); } else { emit ClaimedDaoBounty(proposalAvatar, proposalAvatar, daoBountyReward); } - proposal.daoRedeemedWinnings = true; - } - // If the proposal expires the staked amount is sent back to the staker - else if (proposal.state == ProposalState.Expired) { - transferSuccess = stakingToken.transfer(beneficiary, staked); - - schemes[proposal.schemeId].stakingTokenBalance = schemes[proposal.schemeId].stakingTokenBalance - staked; - - if (!transferSuccess) { - revert VotingMachine__TransferFailed(beneficiary, staked); - } - emit Redeem(proposalId, schemes[proposal.schemeId].avatar, beneficiary, staked); // If the proposal was executed and the stake was in the winning option the beneficiary gets the reward } else if (staker.option == proposal.winningVote) { @@ -426,7 +427,7 @@ contract VotingMachine { if (!transferSuccess) { revert VotingMachine__TransferFailed(beneficiary, reward); } - emit Redeem(proposalId, schemes[proposal.schemeId].avatar, beneficiary, reward); + emit Redeem(proposalId, proposalAvatar, beneficiary, reward); } // If the winning option was yes the reward also include a % (of the staked on the winning option) From f99c903c566ef91975ec61f92bb87e56a1103cfd Mon Sep 17 00:00:00 2001 From: AugustoL Date: Mon, 27 Feb 2023 14:31:19 -0300 Subject: [PATCH 23/24] fix(contracts/dao): cover case when a stake is done on NO by avatar and NO won and avatar redeems --- contracts/dao/votingMachine/VotingMachine.sol | 17 +- test/dao/votingMachines/VotingMachine.js | 150 ++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index f5939b23..3674d095 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -395,7 +395,7 @@ contract VotingMachine { } emit Redeem(proposalId, proposalAvatar, beneficiary, staked); - // If NO won and there is staed tokens on YES, the dao avatar gets a % or the rewards + // If NO won and there is staked tokens on YES, the dao avatar gets a % or the rewards } else if (beneficiary == proposalAvatar && proposal.winningVote == NO && !proposal.daoRedeemedWinnings) { uint256 daoBountyReward = (proposalStakes[proposalId][YES] * parameters[proposal.paramsHash].daoBounty) / proposalStakes[proposalId][NO]; @@ -413,6 +413,21 @@ contract VotingMachine { emit ClaimedDaoBounty(proposalAvatar, proposalAvatar, daoBountyReward); } + // If also a stake was done by the avatar, the stake redeem is done + if (staked > 0) { + reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][NO]; + + schemes[proposal.schemeId].stakingTokenBalance = + schemes[proposal.schemeId].stakingTokenBalance - + reward; + + transferSuccess = stakingToken.transfer(proposalAvatar, reward); + if (!transferSuccess) { + revert VotingMachine__TransferFailed(proposalAvatar, reward); + } + emit Redeem(proposalId, proposalAvatar, proposalAvatar, reward); + } + // If the proposal was executed and the stake was in the winning option the beneficiary gets the reward } else if (staker.option == proposal.winningVote) { // The reward would be a % (of the staked on the winning option) of all the stakes diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index b200b027..73cec621 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -167,6 +167,13 @@ contract("VotingMachine", function (accounts) { constants.MAX_UINT_256, true ); + await permissionRegistry.setETHPermission( + org.avatar.address, + dxdVotingMachine.address, + web3.eth.abi.encodeFunctionSignature("stake(bytes32,uint256,uint256)"), + 0, + true + ); await permissionRegistry.setETHPermission( registrarScheme.address, org.controller.address, @@ -2011,6 +2018,149 @@ contract("VotingMachine", function (accounts) { ); }); + it("Stake on YES by user and stake on NO by avatar, NO wins", async function () { + const testProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [actionMock.address], + [helpers.testCallFrom(org.avatar.address)], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("0.1"), + { + from: accounts[1], + } + ); + + const stakeProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [dxdVotingMachine.address], + [ + web3.eth.abi.encodeFunctionCall( + VotingMachine.abi.find(x => x.name === "stake"), + [testProposalId, constants.NO_OPTION, web3.utils.toWei("0.4")] + ), + ], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + const stakeByAvatarTx = await dxdVotingMachine.vote( + stakeProposalId, + constants.YES_OPTION, + 0, + { + from: accounts[3], + } + ); + await expectEvent.inTransaction( + stakeByAvatarTx.tx, + dxdVotingMachine.contract, + "ProposalExecuteResult", + { 0: "" } + ); + + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("1"), + { + from: accounts[1], + } + ); + const finalVoteTx = await dxdVotingMachine.vote( + testProposalId, + constants.NO_OPTION, + 0, + { + from: accounts[3], + } + ); + + await expectEvent.inTransaction( + finalVoteTx.tx, + dxdVotingMachine.contract, + "StateChange", + { + proposalId: testProposalId, + proposalState: + constants.VOTING_MACHINE_PROPOSAL_STATES.ExecutedInQueue, + } + ); + + const daoBounty = new BN(helpers.defaultParameters.daoBounty); + const stakedOnYes = new BN(web3.utils.toWei("1.1")); + const totalStakedNo = new BN(web3.utils.toWei("0.4")); + const totalStakesWithoutDaoBounty = stakedOnYes.add(totalStakedNo); + + assert( + ( + await dxdVotingMachine.getStaker(testProposalId, org.avatar.address) + ).amount.eq(totalStakedNo) + ); + + const daoBountyRewardToAvatar = stakedOnYes + .mul(daoBounty) + .div(totalStakedNo.add(daoBounty)); + const redeemForStakedOnNo = new BN(web3.utils.toWei("0.4")) + .mul(totalStakesWithoutDaoBounty) + .div(totalStakedNo.add(daoBounty)); + + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + false + ); + + const avatarRedeemTx = await dxdVotingMachine.redeem( + testProposalId, + org.avatar.address + ); + await expectEvent.inTransaction( + avatarRedeemTx.tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: org.avatar.address, + value: daoBountyRewardToAvatar, + } + ); + await expectEvent.inTransaction( + avatarRedeemTx.tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: org.avatar.address, + value: redeemForStakedOnNo, + } + ); + + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + true + ); + + await expectEvent.notEmitted.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, org.avatar.address) + ).tx, + stakingToken.contract, + "Transfer" + ); + }); + it("Stake on multiple proposals in a row and check threshold increase", async function () { const testProposalId1 = await helpers.getValueFromLogs( await masterAvatarScheme.proposeCalls( From deec1aa953663264f80a76fceaa245c21c3e59bb Mon Sep 17 00:00:00 2001 From: AugustoL Date: Wed, 1 Mar 2023 09:24:51 -0300 Subject: [PATCH 24/24] fix(contracts/dao): add missing condition check in reedeem for avatar --- contracts/dao/votingMachine/VotingMachine.sol | 2 +- test/dao/votingMachines/VotingMachine.js | 129 ++++++++++++++++++ 2 files changed, 130 insertions(+), 1 deletion(-) diff --git a/contracts/dao/votingMachine/VotingMachine.sol b/contracts/dao/votingMachine/VotingMachine.sol index 3674d095..885178f1 100644 --- a/contracts/dao/votingMachine/VotingMachine.sol +++ b/contracts/dao/votingMachine/VotingMachine.sol @@ -414,7 +414,7 @@ contract VotingMachine { } // If also a stake was done by the avatar, the stake redeem is done - if (staked > 0) { + if (staked > 0 && staker.option == NO) { reward = (staked * totalStakesWithoutDaoBounty) / proposalStakes[proposalId][NO]; schemes[proposal.schemeId].stakingTokenBalance = diff --git a/test/dao/votingMachines/VotingMachine.js b/test/dao/votingMachines/VotingMachine.js index 73cec621..62400784 100644 --- a/test/dao/votingMachines/VotingMachine.js +++ b/test/dao/votingMachines/VotingMachine.js @@ -2161,6 +2161,135 @@ contract("VotingMachine", function (accounts) { ); }); + it("Stake on YES by user and stake on YES by avatar, NO wins", async function () { + const testProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [actionMock.address], + [helpers.testCallFrom(org.avatar.address)], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("0.1"), + { + from: accounts[1], + } + ); + + const stakeProposalId = await helpers.getValueFromLogs( + await masterAvatarScheme.proposeCalls( + [dxdVotingMachine.address], + [ + web3.eth.abi.encodeFunctionCall( + VotingMachine.abi.find(x => x.name === "stake"), + [testProposalId, constants.YES_OPTION, web3.utils.toWei("0.4")] + ), + ], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "proposalId" + ); + const stakeByAvatarTx = await dxdVotingMachine.vote( + stakeProposalId, + constants.YES_OPTION, + 0, + { + from: accounts[3], + } + ); + await expectEvent.inTransaction( + stakeByAvatarTx.tx, + dxdVotingMachine.contract, + "ProposalExecuteResult", + { 0: "" } + ); + + await dxdVotingMachine.stake( + testProposalId, + constants.YES_OPTION, + web3.utils.toWei("1"), + { + from: accounts[1], + } + ); + const finalVoteTx = await dxdVotingMachine.vote( + testProposalId, + constants.NO_OPTION, + 0, + { + from: accounts[3], + } + ); + + await expectEvent.inTransaction( + finalVoteTx.tx, + dxdVotingMachine.contract, + "StateChange", + { + proposalId: testProposalId, + proposalState: + constants.VOTING_MACHINE_PROPOSAL_STATES.ExecutedInQueue, + } + ); + + const daoBounty = new BN(helpers.defaultParameters.daoBounty); + const stakedOnYes = new BN(web3.utils.toWei("1.5")); + const totalStakedNo = new BN("0"); + + assert( + ( + await dxdVotingMachine.getStaker(testProposalId, org.avatar.address) + ).amount.eq(new BN(web3.utils.toWei("0.4"))) + ); + + const daoBountyRewardToAvatar = stakedOnYes + .mul(daoBounty) + .div(totalStakedNo.add(daoBounty)); + + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + false + ); + + const avatarRedeemTx = await dxdVotingMachine.redeem( + testProposalId, + org.avatar.address + ); + await expectEvent.inTransaction( + avatarRedeemTx.tx, + stakingToken.contract, + "Transfer", + { + from: dxdVotingMachine.address, + to: org.avatar.address, + value: daoBountyRewardToAvatar, + } + ); + + assert.equal( + (await dxdVotingMachine.proposals(testProposalId)).daoRedeemedWinnings, + true + ); + + await expectEvent.notEmitted.inTransaction( + ( + await dxdVotingMachine.redeem(testProposalId, org.avatar.address) + ).tx, + stakingToken.contract, + "Transfer" + ); + }); + it("Stake on multiple proposals in a row and check threshold increase", async function () { const testProposalId1 = await helpers.getValueFromLogs( await masterAvatarScheme.proposeCalls(