Skip to content

Commit

Permalink
Fix skip options on approval voting module (#74)
Browse files Browse the repository at this point in the history
  • Loading branch information
@cairoeth
cairoeth authored Feb 12, 2025
2 parents 31aba44 + 9ea2f2a commit a822b69
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 27 deletions.
5 changes: 3 additions & 2 deletions foundry.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
depth = 16 # The number of times to run the invariant tests
28 changes: 18 additions & 10 deletions src/modules/ApprovalVotingModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}
}
}
Expand Down
62 changes: 47 additions & 15 deletions test/ApprovalVotingModule.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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))
);
}
Expand Down Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand All @@ -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),
Expand Down

0 comments on commit a822b69

Please sign in to comment.