diff --git a/foundry.toml b/foundry.toml index d6bdc27..9da1672 100644 --- a/foundry.toml +++ b/foundry.toml @@ -5,7 +5,8 @@ libs = ['lib'] # A list of library dire optimizer = true # Enable or disable the solc optimizer optimizer_runs = 200 # The number of optimizer runs fs_permissions = [{ access = "read", path = "./"}] # Gives permission to read files for enviroment files. -fail_on_revert = false # Not fail the test if the contract reverts +fail_on_revert = false # Not fail the test if the contract reverts +evm_version = 'cancun' # The EVM version to use [fuzz] runs = 2000 # The number of times to run the fuzzing tests @@ -29,4 +30,4 @@ runs = 10000 # The number of times to run the fuzzing tests [profile.super_deep.invariant] runs = 16 # The number of calls to make in the invariant tests -depth = 16 # The number of times to run the invariant tests \ No newline at end of file +depth = 16 # The number of times to run the invariant tests diff --git a/src/modules/ApprovalVotingModule.sol b/src/modules/ApprovalVotingModule.sol index 47f35a8..204bed1 100644 --- a/src/modules/ApprovalVotingModule.sol +++ b/src/modules/ApprovalVotingModule.sol @@ -126,6 +126,13 @@ contract ApprovalVotingModule is VotingModule { revert InvalidParams(); } + // Enforce that non-zero values use native budget token + for (uint256 n = 0; n < option.targets.length; ++n) { + if (option.values[n] != 0 && proposalSettings.budgetToken != address(0)) { + revert InvalidParams(); + } + } + proposals[proposalId].options.push(option); } } @@ -204,20 +211,21 @@ contract ApprovalVotingModule is VotingModule { { uint256 n; ProposalOption memory option; - bool budgetExceeded = false; // Flatten `options` by filling `executeParams` until budgetAmount is exceeded - for (uint256 i; i < succeededOptionsLength;) { + for (uint256 i; i < succeededOptionsLength; ++i) { + bool budgetExceeded = false; option = sortedOptions[i]; + uint256 optionTotalValue = 0; for (n = 0; n < option.targets.length;) { - // If `budgetToken` is ETH and value is not zero, add transaction value to `totalValue` + // If `budgetToken` is ETH and value is not zero, add transaction value to `optionTotalValue` if (settings.budgetToken == address(0) && option.values[n] != 0) { - if (totalValue + option.values[n] > settings.budgetAmount) { + if (totalValue + optionTotalValue + option.values[n] > settings.budgetAmount) { budgetExceeded = true; break; // break inner loop } - totalValue += option.values[n]; + optionTotalValue += option.values[n]; } unchecked { @@ -229,20 +237,20 @@ contract ApprovalVotingModule is VotingModule { } // If `budgetAmount` for ETH is exceeded, skip option. - if (budgetExceeded) break; + if (budgetExceeded) continue; // Check if budgetAmount is exceeded for non-ETH tokens if (settings.budgetToken != address(0) && settings.budgetAmount != 0) { if (option.budgetTokensSpent != 0) { - if (totalValue + option.budgetTokensSpent > settings.budgetAmount) break; // break outer loop for non-ETH tokens - totalValue += option.budgetTokensSpent; + if (totalValue + optionTotalValue + option.budgetTokensSpent > settings.budgetAmount) continue; // break outer loop for non-ETH tokens + optionTotalValue += option.budgetTokensSpent; } } + // Add option-specific value to the total value + totalValue += optionTotalValue; unchecked { executeParamsLength += n; - - ++i; } } } diff --git a/test/ApprovalVotingModule.t.sol b/test/ApprovalVotingModule.t.sol index f0d907b..1d0cbad 100644 --- a/test/ApprovalVotingModule.t.sol +++ b/test/ApprovalVotingModule.t.sol @@ -375,9 +375,8 @@ contract ApprovalVotingModuleTest is Test { uint256 proposalId = hashProposalWithModule(governor, address(module), proposalData, descriptionHash); module.propose(proposalId, proposalData, descriptionHash); - uint256[] memory votes = new uint256[](2); + uint256[] memory votes = new uint256[](1); votes[0] = 1; - votes[1] = 2; bytes memory params = abi.encode(votes); module._countVote(proposalId, voter, uint8(VoteType.For), weight, params); @@ -396,13 +395,10 @@ contract ApprovalVotingModuleTest is Test { assertEq(targets[0], options[1].targets[0]); assertEq(values[0], options[1].values[0]); assertEq(calldatas[0], options[1].calldatas[0]); - assertEq(targets[1], options[1].targets[1]); - assertEq(values[1], options[1].values[1]); - assertEq(calldatas[1], options[1].calldatas[1]); - assertEq(targets[2], address(module)); - assertEq(values[2], 0); + assertEq(targets[1], address(module)); + assertEq(values[1], 0); assertEq( - calldatas[2], + calldatas[1], abi.encodeCall(ApprovalVotingModule._afterExecute, (proposalId, proposalData, options[1].budgetTokensSpent)) ); } @@ -625,6 +621,37 @@ contract ApprovalVotingModuleTest is Test { module._formatExecuteParams(proposalId, proposalData); } + function testReverts_formatExecuteParams_nativeMix() public { + address[] memory _targets = new address[](2); + uint256[] memory _values = new uint256[](2); + bytes[] memory _calldatas = new bytes[](2); + // Token transfer + _targets[0] = token; + _values[0] = 0; + _calldatas[0] = abi.encodeCall(IERC20.transfer, (receiver1, 100)); + // Native transfer + _targets[1] = receiver1; + _values[1] = 0.01 ether; + + ProposalOption[] memory options = new ProposalOption[](1); + options[0] = ProposalOption(100, _targets, _values, _calldatas, "option 1"); + + ProposalSettings memory settings = ProposalSettings({ + maxApprovals: 2, + criteria: uint8(PassingCriteria.TopChoices), + criteriaValue: 1, + budgetToken: token, + budgetAmount: 100 + }); + + bytes memory proposalData = abi.encode(options, settings); + + vm.startPrank(governor); + uint256 proposalId = hashProposalWithModule(governor, address(module), proposalData, descriptionHash); + vm.expectRevert(VotingModule.InvalidParams.selector); + module.propose(proposalId, proposalData, descriptionHash); + } + /*////////////////////////////////////////////////////////////// HELPERS //////////////////////////////////////////////////////////////*/ @@ -647,22 +674,27 @@ contract ApprovalVotingModuleTest is Test { // Transfer 100 OP tokens to receiver2 targets2[0] = token; calldatas2[0] = abi.encodeCall(IERC20.transfer, (receiver1, budgetExceeded ? 6e17 : 100)); - // Send 0.01 ether to receiver2, and emit call to test calls to targets different than budgetTokens are ignored targets2[1] = receiver2; - values2[1] = budgetExceeded ? 0.6 ether : 0.01 ether; + values2[1] = (!isBudgetOp && budgetExceeded) ? 0.6 ether : 0; calldatas2[1] = calldatas2[0]; - options = new ProposalOption[](3); - options[0] = ProposalOption(0, targets1, values1, calldatas1, "option 1"); - options[1] = ProposalOption(budgetExceeded ? 6e17 : 100, targets2, values2, calldatas2, "option 2"); - address[] memory targets3 = new address[](1); uint256[] memory values3 = new uint256[](1); bytes[] memory calldatas3 = new bytes[](1); targets3[0] = token; calldatas3[0] = abi.encodeCall(IERC20.transferFrom, (address(governor), receiver1, budgetExceeded ? 6e17 : 100)); - options[2] = ProposalOption(budgetExceeded ? 6e17 : 100, targets3, values3, calldatas3, "option 3"); + if (isBudgetOp) { + options = new ProposalOption[](2); + options[0] = ProposalOption(budgetExceeded ? 6e17 : 100, targets2, values2, calldatas2, "option 2"); + options[1] = ProposalOption(budgetExceeded ? 6e17 : 100, targets3, values3, calldatas3, "option 3"); + } else { + options = new ProposalOption[](3); + options[0] = ProposalOption(0, targets1, values1, calldatas1, "option 1"); + options[1] = ProposalOption(budgetExceeded ? 6e17 : 100, targets2, values2, calldatas2, "option 2"); + options[2] = ProposalOption(budgetExceeded ? 6e17 : 100, targets3, values3, calldatas3, "option 3"); + } + settings = ProposalSettings({ maxApprovals: 2, criteria: uint8(PassingCriteria.TopChoices),