Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix skip options on approval voting module #74

Open
wants to merge 4 commits into
base: dg-audit
Choose a base branch
from
Open

Fix skip options on approval voting module #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31c3add
Add EVM version to toml (#68)
cairoeth Dec 8, 2024
6d5c2ff
Fix skip options
cairoeth Jan 13, 2025
4ff902d
Add option-specific value tracker
cairoeth Jan 16, 2025
9ea2f2a
Check for budget token and value
cairoeth Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions foundry.toml
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,9 +376,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 @@ -397,13 +396,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 @@ -626,6 +622,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 @@ -648,22 +675,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