Update remediations (#70)

* prompt proposal id * add deploy code * add make persistent * Check calldata * add run function with flags back * bravo match working * remove run function with flags * Bravo check calldata integration test * Timelock check onchain calldata * delete validate calldata script * Comments * Run tests on sepolia fork * move addresses test to a separate CI * update Addresses.json * remove default checkOnChainCalldata implementation * lint * debug * ci debug * Update Addresses.json * redeploy timelock * move from internal to public functions * functions visibility * print functions * review changes * rename DEV address to DEPLOYER_EOA * rename ci test * Remove examples and integration tests from this repo * remove calldata generation from CI * remove run-proposal.sh * merge conflicts * TimelockProposalUnitTest test_build pass * update ci * update foundry.toml * after deploy mock * fix state diff recording * test simulate working * Remove caller from Proposal constructor * test getCalldata * getExecuteCalldata * mock MockTimelockProposal * bravo mocks * setTimelock and setAddresses * Test Bravo Proposal * timelock description * fix timelock test_getCalldata * lint * compile * MockMultisigProposal * Multisig tests * add checkOnChainCalldata tests * do not broadcast afterDeployMock call * add validate function tests * remediate addresses.t.sol * unit -> integration tests * after deploy flag * remove multisig cast * simplify vault * simplify Vault and Token mocks * remove bravo mocks * lint * interfaces * delete interfaces/IGovernorBravo * compound proposal * Compound test * debug * run CI on mainnet * lint * add validateActionInclusion internal function * natspec * action hash * review changes * remove params from _simulateActions * Multisig Proposal example using Optmism * fix validate function * lint * Delete TimelockProposal * fix failing tests * review changes * compile
solidity-labs-io · May 14, 2024 · 05ecdd8 · 05ecdd8
1 parent ef53962
commit 05ecdd8
Show file tree

Hide file tree

Showing 40 changed files with 1,397 additions and 1,924 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -43,7 +43,7 @@ jobs:
                     echo "✅ Passed or warnings found" >> $GITHUB_STEP_SUMMARY
                   fi
 
-    unit-test:
+    integration-test:
         runs-on: "ubuntu-latest"
         steps:
             - name: "Check out the repo"
@@ -68,8 +68,8 @@ jobs:
             - name: "Build the contracts and print their size"
               run: "forge build --sizes"
 
-            - name: "Proposal Simulator Test"
-              run: "forge test --nmc '(TypeCheck|TestAddresses)'"
+            - name: "Proposal Simulator Integration Tests"
+              run: "forge test --mc IntegrationTest --fork-url mainnet"
 
             - name: "Add test summary"
               run: |

diff --git a/addresses/Addresses.json b/addresses/Addresses.json
@@ -1,32 +1,14 @@
 [
     {
-        "addr": "0x3dd46846eed8D147841AE162C8425c08BD8E1b41",
-        "name": "DEV_MULTISIG",
-        "chainId": 31337,
-        "isContract": false
-    },
-    {
-        "addr": "0x7da82C7AB4771ff031b66538D2fB9b0B047f6CF9",
-        "name": "TEAM_MULTISIG",
-        "chainId": 31337,
-        "isContract": false
-    },
-    {
-        "addr": "0x1a9C8182C09F50C8318d769245beA52c32BE35BC",
-        "name": "PROTOCOL_TIMELOCK",
-        "chainId": 31337,
-        "isContract": false
-    },
-    {
-        "addr": "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266",
-        "name": "PROTOCOL_GOVERNOR",
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA",
         "chainId": 31337,
         "isContract": false
     },
     {
-        "addr": "0x70997970C51812dc3A010C7d01b50e0d17dc79C8",
-        "name": "PROTOCOL_GOVERNANCE_TOKEN",
-        "chainId": 31337,
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA",
+        "chainId": 1,
         "isContract": false
     },
     {
@@ -41,17 +23,47 @@
         "chainId": 11155111,
         "isContract": false
     },
-    {
-        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
-        "name": "DEPLOYER_EOA",
-        "chainId": 31337,
-        "isContract": false
-    },
     {
         "addr": "0x2EE28b12156fCaa13Bb188347cE6635C18bc94ef",
         "chainId": 11155111,
         "isContract": true,
-        "name": "PROTOCOL_GOVERNOR"
+        "name": "GOVERNOR_BRAVO"
+    },
+    {
+        "addr": "0xc0da02939e1441f497fd74f78ce7decb17b66529",
+        "chainId": 1,
+        "isContract": true,
+        "name": "COMPOUND_GOVERNOR_BRAVO"
+    },
+    {
+        "addr": "0x316f9708bB98af7dA9c68C1C3b5e79039cD336E3",
+        "chainId": 1,
+        "isContract": true,
+        "name": "COMPOUND_CONFIGURATOR"
+    },
+    {
+        "addr": "0xf603265f91f58F1EfA4fAd57694Fb3B77b25fC18",
+        "chainId": 1,
+        "isContract": false,
+        "name": "COMPOUND_PROPOSER"
+    },
+    {
+        "addr": "0xa17581a9e3356d9a858b789d68b4d866e593ae94",
+        "chainId": 1,
+        "isContract": true,
+        "name": "COMPOUND_COMET"
+    },
+    {
+        "addr": "0xc00e94cb662c3520282e6f5717214004a7f26888",
+        "chainId": 1,
+        "isContract": true,
+        "name": "COMP_TOKEN"
+    },
+    {
+        "addr": "0x6d903f6003cca6255D85CcA4D3B5E5146dC33925",
+        "chainId": 1,
+        "isContract": true,
+        "name": "COMPOUND_TIMELOCK_BRAVO"
     },
     {
         "addr": "0x37039662a2364Df8206691c1D1b9ff8d22389290",
@@ -66,7 +78,7 @@
         "name": "PROTOCOL_GOVERNANCE_TOKEN"
     },
     {
-        "addr": "0xb7dd5fd38c6dc6af3dded96bbd8935a71eb04b21",
+        "addr": "0xAE96E645c7c7628BA9507B1e91A5fA3Cb1EB5DA1",
         "chainId": 11155111,
         "isContract": true,
         "name": "PROTOCOL_TIMELOCK"
@@ -75,7 +87,31 @@
         "addr": "0x35e292eA59C71caC368ecdf9046eF7C3ED0DF79E",
         "chainId": 11155111,
         "isContract": true,
-        "name": "PROTOCOL_GOVERNOR_ALPHA"
+        "name": "GOVERNOR_BRAVO_ALPHA"
+    },
+    {
+        "addr": "0x7D26D5978cD4617C6c7a02C75a42927eF52f405C",
+        "chainId": 11155111,
+        "isContract": true,
+        "name": "TIMELOCK_VAULT"
+    },
+    {
+        "addr": "0x861bB12491F453F48fE1A05e9C034588de638262",
+        "chainId": 11155111,
+        "isContract": true,
+        "name": "TIMELOCK_TOKEN"
+    },
+    {
+        "addr": "0x0CB10d561C2a273AFBB81E535738BA3095E9dCF2",
+        "chainId": 11155111,
+        "isContract": true,
+        "name": "BRAVO_VAULT"
+    },
+    {
+        "addr": "0x76126BDFC0893d9A27336b93824778432cA60808",
+        "chainId": 11155111,
+        "isContract": true,
+        "name": "BRAVO_VAULT_TOKEN"
     },
     {
         "addr": "0x293cad26033577eb68137603a34d2dbfd05104d8",
@@ -90,15 +126,27 @@
         "isContract": true
     },
     {
-        "addr": "0x9eF929330b3ABB6A814DF726A5834D43Df093E2d",
-        "chainId": 11155111,
-        "isContract": true,
-        "name": "VAULT"
+        "addr": "0x5a0Aae59D09fccBdDb6C6CcEB07B7279367C3d2A",
+        "chainId": 1,
+        "name": "OPTIMISM_MULTISIG",
+        "isContract": true
     },
     {
-        "addr": "0x8dfF5C5b38d65b36064226719F4839E288411376",
-        "chainId": 11155111,
-        "isContract": true,
-        "name": "TOKEN_1"
+        "addr": "0x543bA4AADBAb8f9025686Bd03993043599c6fB04",
+        "chainId": 1,
+        "name": "OPTIMISM_PROXY_ADMIN",
+        "isContract": true
+    },
+    {
+        "addr": "0x5a7749f83b81b301cab5f48eb8516b986daef23d",
+        "chainId": 1,
+        "name": "OPTIMISM_L1_NFT_BRIDGE_PROXY",
+        "isContract": true
+    },
+    {
+        "addr": "0xAE2AF01232a6c4a4d3012C5eC5b1b35059caF10d",
+        "chainId": 1,
+        "name": "OPTIMISM_L1_NFT_BRIDGE_IMPLEMENTATION",
+        "isContract": true
     }
 ]
diff --git a/addresses/Addresses.sol b/addresses/Addresses.sol
@@ -278,8 +278,8 @@ contract Addresses is IAddresses, Test {
         return _addresses[name][chainId].addr != address(0);
     }
 
-    /// @dev Print recorded addresses
-    function printRecordedAddresses() external view {
+    /// @dev Print new recorded and changed addresses
+    function printJSONChanges() external view {
         {
             (
                 string[] memory names,

diff --git a/addresses/AddressesDuplicated.json b/addresses/AddressesDuplicated.json
@@ -1,13 +1,13 @@
 [
     {
-        "addr": "0x3dd46846eed8D147841AE162C8425c08BD8E1b41",
-        "name": "DEV_MULTISIG",
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA",
         "chainId": 31337,
         "isContract": false
     },
     {
-        "addr": "0x3dd46846eed8D147841AE162C8425c08BD8E1b41",
-        "name": "DEV_MULTISIG",
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA",
         "chainId": 31337,
         "isContract": false
     }

diff --git a/addresses/AddressesDuplicatedDifferentName.json b/addresses/AddressesDuplicatedDifferentName.json
@@ -1,13 +1,13 @@
 [
     {
-        "addr": "0x3dd46846eed8D147841AE162C8425c08BD8E1b41",
-        "name": "DEV_MULTISIG",
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA",
         "chainId": 31337,
         "isContract": false
     },
     {
-        "addr": "0x3dd46846eed8D147841AE162C8425c08BD8E1b41",
-        "name": "DEV_MULTISIG2",
+        "addr": "0x9679E26bf0C470521DE83Ad77BB1bf1e7312f739",
+        "name": "DEPLOYER_EOA2",
         "chainId": 31337,
         "isContract": false
     }

diff --git a/docs/assets/diagram.svg b/docs/assets/diagram.svg
diff --git a/docs/guides/governor-bravo-proposal.md b/docs/guides/governor-bravo-proposal.md
@@ -39,7 +39,7 @@ contract BRAVO_01 is GovernorBravoProposal {
     }
 
     /// @notice Deploys a vault contract and an ERC20 token contract.
-    function _deploy() internal override {
+    function deploy() public override {
         if (!addresses.isAddressSet("VAULT")) {
             Vault timelockVault = new Vault();
             addresses.addAddress("VAULT", address(timelockVault), true);
@@ -55,7 +55,7 @@ contract BRAVO_01 is GovernorBravoProposal {
     /// 1. Transfers vault ownership to timelock.
     /// 2. Transfer token ownership to timelock.
     /// 3. Transfers all tokens to timelock.
-    function _afterDeploy() internal override {
+    function afterDeployMock() public override {
         address timelock = addresses.getAddress("PROTOCOL_TIMELOCK");
         Vault timelockVault = Vault(addresses.getAddress("VAULT"));
         MockToken token = MockToken(addresses.getAddress("TOKEN_1"));
@@ -71,7 +71,7 @@ contract BRAVO_01 is GovernorBravoProposal {
     }
 
     /// @notice Sets up actions for the proposal, in this case, setting the MockToken to active.
-    function _build() internal override {
+    function build() public override {
         /// STATICCALL -- not recorded for the run stage
         address timelockVault = addresses.getAddress("VAULT");
         address token = addresses.getAddress("TOKEN_1");
@@ -81,19 +81,19 @@ contract BRAVO_01 is GovernorBravoProposal {
     }
 
     /// @notice Executes the proposal actions.
-    function _run() internal override {
+    function simulate() public override {
         // Call parent _run function to check if there are actions to execute
-        super._run();
+        super.simulate();
 
-        address governor = addresses.getAddress("PROTOCOL_GOVERNOR");
+        address governor = addresses.getAddress("GOVERNOR_BRAVO");
         address govToken = addresses.getAddress("PROTOCOL_GOVERNANCE_TOKEN");
         address proposer = addresses.getAddress("DEPLOYER_EOA");
 
         _simulateActions(governor, govToken, proposer);
     }
 
     /// @notice Validates the post-execution state.
-    function _validate() internal override {
+    function validate() public override {
         address timelock = addresses.getAddress("PROTOCOL_TIMELOCK");
         Vault timelockVault = Vault(addresses.getAddress("VAULT"));
         MockToken token = MockToken(addresses.getAddress("TOKEN_1"));
@@ -117,7 +117,7 @@ Let's go through each of the functions that are overridden.
     they are added to the `Addresses` contract by calling `addAddress()`.
 -   `_build()`: Set the necessary actions for your proposal. In this example, ERC20 token is whitelisted on the Vault contract. The actions should be
     written in solidity code and in the order they should be executed. Any calls (except to the Addresses object) will be recorded and stored as actions to execute in the run function.
--   `_run()`: Execute the proposal actions outlined in the `_build()` step. This
+-   `simulate()`: Execute the proposal actions outlined in the `_build()` step. This
     function performs a call to `_simulateActions` from the inherited
     `GovernorBravoProposal` contract. Internally, `_simulateActions()` uses the calldata generated from the actions set up in the build step, and simulates the end-to-end workflow of a successful proposal submission, starting with a call to [propose](https://github.com/compound-finance/compound-governance/blob/5e581ef817464fdb71c4c7ef6bde4c552302d160/contracts/GovernorBravoDelegate.sol#L118).
 -   `_validate()`: This final step is crucial for validating the post-execution state. It ensures that the timelock is the new owner of Vault and token, the tokens were transferred to timelock and the token was whitelisted on the Vault contract
@@ -175,7 +175,7 @@ Copy the addresses of the timelock, governor, and governance token from the scri
     },
     {
         "addr": "YOUR_GOVERNOR_ADDRESS",
-        "name": "PROTOCOL_GOVERNOR",
+        "name": "GOVERNOR_BRAVO",
         "chainId": 11155111,
         "isContract": true
     },
@@ -205,7 +205,7 @@ Run the script:
 forge script script/InitializeBravo.s.sol --rpc-url sepolia --broadcast -vvvv --slow --sender ${wallet_address} -vvvv --account ${wallet_name} -g 200
 ```
 
-Copy the _PROTOCOL_GOVERNOR_ALPHA_ address from the script output and add it to
+Copy the _GOVERNOR_BRAVO_ALPHA_ address from the script output and add it to
 the `Addresses.json` file.
 
 ### Setting Up the Addresses JSON
@@ -223,7 +223,7 @@ to Address.json. The final Address.json file should be something like this:
     },
     {
         "addr": "YOUR_GOVERNOR_ADDRESS",
-        "name": "PROTOCOL_GOVERNOR",
+        "name": "GOVERNOR_BRAVO",
         "chainId": 11155111,
         "isContract": true
     },
@@ -235,7 +235,7 @@ to Address.json. The final Address.json file should be something like this:
     },
     {
         "addr": "YOUR_GOVERNOR_ALPHA_ADDRESS",
-        "name": "PROTOCOL_GOVERNOR_ALPHA",
+        "name": "GOVERNOR_BRAVO_ALPHA",
         "chainId": 11155111,
         "isContract": true
     },

diff --git a/docs/guides/multisig-proposal.md b/docs/guides/multisig-proposal.md
@@ -39,7 +39,7 @@ contract MULTISIG_01 is MultisigProposal {
     }
 
     /// @notice Deploys a vault contract and an ERC20 token contract.
-    function _deploy() internal override {
+    function deploy() public override {
         if (!addresses.isAddressSet("VAULT")) {
             Vault timelockVault = new Vault();
             addresses.addAddress("VAULT", address(timelockVault), true);
@@ -55,7 +55,7 @@ contract MULTISIG_01 is MultisigProposal {
     /// 1. Transfers vault ownership to dev multisig.
     /// 2. Transfer token ownership to dev multisig.
     /// 3. Transfers all tokens to dev multisig.
-    function _afterDeploy() internal override {
+    function afterDeployMock() public override {
         address devMultisig = addresses.getAddress("DEV_MULTISIG");
         Vault timelockVault = Vault(addresses.getAddress("VAULT"));
         MockToken token = MockToken(addresses.getAddress("TOKEN_1"));
@@ -70,7 +70,7 @@ contract MULTISIG_01 is MultisigProposal {
     }
 
     /// @notice Sets up actions for the proposal, in this case, setting the MockToken to active.
-    function _build() internal override {
+    function build() public override {
         /// STATICCALL -- not recorded for the run stage
         address timelockVault = addresses.getAddress("VAULT");
         address token = addresses.getAddress("TOKEN_1");
@@ -80,9 +80,9 @@ contract MULTISIG_01 is MultisigProposal {
     }
 
     /// @notice Executes the proposal actions.
-    function _run() internal override {
+    function simulate() public override {
         /// Call parent _run function to check if there are actions to execute
-        super._run();
+        super.simulate();
 
         address multisig = addresses.getAddress("DEV_MULTISIG");
 
@@ -91,7 +91,7 @@ contract MULTISIG_01 is MultisigProposal {
     }
 
     /// @notice Validates the post-execution state.
-    function _validate() internal override {
+    function validate() public override {
         address devMultisig = addresses.getAddress("DEV_MULTISIG");
         Vault timelockVault = Vault(addresses.getAddress("VAULT"));
         MockToken token = MockToken(addresses.getAddress("TOKEN_1"));
@@ -116,7 +116,7 @@ Let's go through each of the functions that are overridden.
 -   `_build()`: Set the necessary actions for your proposal. In this example,
     ERC20 token is whitelisted on the Vault contract. The actions should be
     written in solidity code and in the order they should be executed. Any calls (except to the Addresses object) will be recorded and stored as actions to execute in the run function.
--   `_run()`: Execute the proposal actions outlined in the `_build()` step. This
+-   `simulate()`: Execute the proposal actions outlined in the `_build()` step. This
     function performs a call to `simulateActions()` from the inherited
     `MultisigProposal` contract. Internally, `_simulateActions()` simulates a call to the [Multicall3](https://www.multicall3.com/) contract with the calldata generated from the actions set up in the build step.
 -   `_validate()`: This final step is crucial for validating the post-execution state. It ensures that the multisig is the new owner of Vault and token, the tokens were transferred to multisig and the token was whitelisted on the Vault contract