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.

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

Audit fix: Proposer is not saved proposal data #205

Merged
merged 3 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion contracts/DualGovernance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ contract DualGovernance is IDualGovernance {
revert ProposalSubmissionBlocked();
}
Proposers.Proposer memory proposer = _proposers.getProposer(msg.sender);
proposalId = TIMELOCK.submit(proposer.executor, calls, metadata);
proposalId = TIMELOCK.submit(proposer.account, proposer.executor, calls, metadata);
}

/// @notice Schedules a previously submitted proposal for execution in the Dual Governance system.
Expand Down
4 changes: 3 additions & 1 deletion contracts/EmergencyProtectedTimelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,19 @@ contract EmergencyProtectedTimelock is IEmergencyProtectedTimelock {
// ---

/// @notice Submits a new proposal to execute a series of calls through an executor.
/// @param proposer The address of the proposer submitting the proposal.
/// @param executor The address of the executor contract that will execute the calls.
/// @param calls An array of `ExternalCall` structs representing the calls to be executed.
/// @param metadata A string containing additional information about the proposal.
/// @return newProposalId The id of the newly created proposal.
function submit(
address proposer,
address executor,
ExternalCall[] calldata calls,
string calldata metadata
) external returns (uint256 newProposalId) {
_timelockState.checkCallerIsGovernance();
newProposalId = _proposals.submit(executor, calls, metadata);
newProposalId = _proposals.submit(proposer, executor, calls, metadata);
}

/// @notice Schedules a proposal for execution after a specified delay.
Expand Down
2 changes: 1 addition & 1 deletion contracts/TimelockedGovernance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ contract TimelockedGovernance is IGovernance {
string calldata metadata
) external returns (uint256 proposalId) {
_checkCallerIsGovernance();
return TIMELOCK.submit(TIMELOCK.getAdminExecutor(), calls, metadata);
return TIMELOCK.submit(msg.sender, TIMELOCK.getAdminExecutor(), calls, metadata);
}

/// @notice Schedules a submitted proposal.
Expand Down
1 change: 1 addition & 0 deletions contracts/interfaces/ITimelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ interface ITimelock {
}

function submit(
address proposer,
address executor,
ExternalCall[] calldata calls,
string calldata metadata
Expand Down
8 changes: 6 additions & 2 deletions contracts/libraries/ExecutableProposals.sol
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ library ExecutableProposals {
// Events
// ---

event ProposalSubmitted(uint256 indexed id, address indexed executor, ExternalCall[] calls, string metadata);
event ProposalSubmitted(
uint256 indexed id, address indexed proposer, address indexed executor, ExternalCall[] calls, string metadata
);
event ProposalScheduled(uint256 indexed id);
event ProposalExecuted(uint256 indexed id, bytes[] callResults);
event ProposalsCancelledTill(uint256 proposalId);
Expand All @@ -102,12 +104,14 @@ library ExecutableProposals {

/// @notice Submits a new proposal with the specified executor and external calls.
/// @param self The context of the Executable Proposal library.
/// @param proposer The address of the proposer submitting the proposal.
/// @param executor The address authorized to execute the proposal.
/// @param calls The list of external calls to include in the proposal.
/// @param metadata Metadata describing the proposal.
/// @return newProposalId The id of the newly submitted proposal.
function submit(
Context storage self,
address proposer,
bulbozaur marked this conversation as resolved.
Show resolved Hide resolved
address executor,
ExternalCall[] memory calls,
string memory metadata
Expand All @@ -129,7 +133,7 @@ library ExecutableProposals {
newProposal.calls.push(calls[i]);
}

emit ProposalSubmitted(newProposalId, executor, calls, metadata);
emit ProposalSubmitted(newProposalId, proposer, executor, calls, metadata);
}

/// @notice Marks a previously submitted proposal as scheduled for execution if the required delay period
Expand Down
2 changes: 1 addition & 1 deletion docs/plan-b.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ If the Emergency Committees are set up and active, the governance proposal under
While active, the Emergency Activation Committee can enable Emergency Mode. This mode prohibits anyone but the Emergency Execution Committee from executing proposals. Once the **Emergency Duration** has ended, the Emergency Execution Committee or anyone else may disable the emergency mode, canceling all pending proposals. After the emergency mode is deactivated or the Emergency Period has elapsed, the Emergency Committees lose their power.
### Function: `EmergencyProtectedTimelock.submit`
```solidity
function submit(address executor, ExecutorCall[] calls, string metadata)
function submit(address proposer, address executor, ExecutorCall[] calls, string metadata)
returns (uint256 proposalId)
```
Registers a new governance proposal composed of one or more external `calls` to be made by the `executor` contract. Initiates the `AfterSubmitDelay`.
Expand Down
2 changes: 1 addition & 1 deletion docs/specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ The governance reset entails the following steps:
### Function: EmergencyProtectedTimelock.submit

```solidity
function submit(address executor, ExecutorCall[] calls, string calldata metadata)
function submit(address proposer, address executor, ExecutorCall[] calls, string calldata metadata)
bulbozaur marked this conversation as resolved.
Show resolved Hide resolved
returns (uint256 proposalId)
```

Expand Down
7 changes: 6 additions & 1 deletion test/mocks/TimelockMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ contract TimelockMock is ITimelock {

address internal governance;

function submit(address, ExternalCall[] calldata, string calldata) external returns (uint256 newProposalId) {
function submit(
address,
address,
ExternalCall[] calldata,
string calldata
) external returns (uint256 newProposalId) {
newProposalId = submittedProposals.length + OFFSET;
submittedProposals.push(newProposalId);
canScheduleProposal[newProposalId] = false;
Expand Down
6 changes: 4 additions & 2 deletions test/unit/DualGovernance.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ contract DualGovernanceUnitTests is UnitTest {
ExternalCall[] memory calls = _generateExternalCalls();
Proposers.Proposer memory proposer = _dualGovernance.getProposer(address(this));
vm.expectCall(
address(_timelock), 0, abi.encodeWithSelector(TimelockMock.submit.selector, proposer.executor, calls, "")
address(_timelock),
0,
abi.encodeWithSelector(TimelockMock.submit.selector, address(this), proposer.executor, calls, "")
);

uint256 proposalId = _dualGovernance.submitProposal(calls, "");
Expand Down Expand Up @@ -2090,7 +2092,7 @@ contract DualGovernanceUnitTests is UnitTest {

function _submitMockProposal() internal {
// mock timelock doesn't uses proposal data
_timelock.submit(address(0), new ExternalCall[](0), "");
_timelock.submit(msg.sender, address(0), new ExternalCall[](0), "");
}

function _generateExternalCalls() internal pure returns (ExternalCall[] memory calls) {
Expand Down
32 changes: 22 additions & 10 deletions test/unit/EmergencyProtectedTimelock.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {EmergencyProtection} from "contracts/libraries/EmergencyProtection.sol";

import {Executor} from "contracts/Executor.sol";
import {EmergencyProtectedTimelock, TimelockState} from "contracts/EmergencyProtectedTimelock.sol";
import {ExecutableProposals} from "contracts/libraries/ExecutableProposals.sol";

import {UnitTest} from "test/utils/unit-test.sol";
import {TargetMock} from "test/utils/target-mock.sol";
Expand All @@ -22,6 +23,7 @@ import {ExternalCall} from "test/utils/executor-calls.sol";
contract EmergencyProtectedTimelockUnitTests is UnitTest {
EmergencyProtectedTimelock private _timelock;
TargetMock private _targetMock;
TargetMock private _anotherTargetMock;
Executor private _executor;

address private _emergencyActivator = makeAddr("EMERGENCY_ACTIVATION_COMMITTEE");
Expand All @@ -40,6 +42,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
_timelock = _deployEmergencyProtectedTimelock();

_targetMock = new TargetMock();
_anotherTargetMock = new TargetMock();

_executor.transferOwnership(address(_timelock));

Expand Down Expand Up @@ -77,13 +80,21 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {

vm.prank(stranger);
vm.expectRevert(abi.encodeWithSelector(TimelockState.CallerIsNotGovernance.selector, [stranger]));
_timelock.submit(_adminExecutor, new ExternalCall[](0), "");
_timelock.submit(stranger, _adminExecutor, new ExternalCall[](0), "");
assertEq(_timelock.getProposalsCount(), 0);
}

function test_submit_HappyPath() external {
string memory testMetadata = "testMetadata";

vm.expectEmit(true, true, true, true);
emit ExecutableProposals.ProposalSubmitted(
1, _dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), testMetadata
);
vm.prank(_dualGovernance);
_timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
_timelock.submit(
_dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), testMetadata
);

assertEq(_timelock.getProposalsCount(), 1);

Expand Down Expand Up @@ -357,7 +368,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {

function test_emergencyExecute_RevertOn_ModeNotActive() external {
vm.startPrank(_dualGovernance);
_timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
_timelock.submit(_dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");

assertEq(_timelock.getProposalsCount(), 1);

Expand Down Expand Up @@ -808,8 +819,9 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {

vm.startPrank(_dualGovernance);
ExternalCall[] memory executorCalls = _getMockTargetRegularStaffCalls(address(_targetMock));
_timelock.submit(_adminExecutor, executorCalls, "");
_timelock.submit(_adminExecutor, executorCalls, "");
ExternalCall[] memory anotherExecutorCalls = _getMockTargetRegularStaffCalls(address(_anotherTargetMock));
_timelock.submit(_dualGovernance, _adminExecutor, executorCalls, "");
bulbozaur marked this conversation as resolved.
Show resolved Hide resolved
_timelock.submit(_dualGovernance, _adminExecutor, anotherExecutorCalls, "");

(ITimelock.ProposalDetails memory submittedProposal, ExternalCall[] memory calls) = _timelock.getProposal(1);

Expand Down Expand Up @@ -876,9 +888,9 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
// assertEq(cancelledProposal.executedAt, Timestamps.ZERO);
// assertEq doesn't support comparing enumerables so far
assertEq(cancelledCalls.length, 1);
assertEq(cancelledCalls[0].value, executorCalls[0].value);
assertEq(cancelledCalls[0].target, executorCalls[0].target);
assertEq(cancelledCalls[0].payload, executorCalls[0].payload);
assertEq(cancelledCalls[0].value, anotherExecutorCalls[0].value);
assertEq(cancelledCalls[0].target, anotherExecutorCalls[0].target);
assertEq(cancelledCalls[0].payload, anotherExecutorCalls[0].payload);
}

function test_get_not_existing_proposal() external {
Expand Down Expand Up @@ -966,7 +978,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
function test_getProposalCalls() external {
ExternalCall[] memory executorCalls = _getMockTargetRegularStaffCalls(address(_targetMock));
vm.prank(_dualGovernance);
_timelock.submit(_adminExecutor, executorCalls, "");
_timelock.submit(_dualGovernance, _adminExecutor, executorCalls, "");

ExternalCall[] memory calls = _timelock.getProposalCalls(1);

Expand Down Expand Up @@ -994,7 +1006,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {

function _submitProposal() internal {
vm.prank(_dualGovernance);
_timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
_timelock.submit(_dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
}

function _scheduleProposal(uint256 proposalId) internal {
Expand Down
Loading