Merge pull request #34 from bugout-dev/hotfix-security-fix

Pool creation can only be done by controller now.
2022-02-01 07:10:49 -08:00 · 2022-02-01 07:10:49 -08:00 · dfa1cc5bcb
commit dfa1cc5bcb
--- a/contracts/terminus/TerminusFacet.sol
+++ b/contracts/terminus/TerminusFacet.sol
@ -39,6 +39,11 @@ contract TerminusFacet is ERC1155WithTerminusStorage {
        uint256[] amounts
    );

+    function setController(address newController) external {
+        LibTerminus.enforceIsController();
+        LibTerminus.setController(newController);
+    }
+
    function poolMintBatch(
        uint256 id,
        address[] memory toAddresses,
@ -172,6 +177,7 @@ contract TerminusFacet is ERC1155WithTerminusStorage {
    }

    function createSimplePool(uint256 _capacity) external returns (uint256) {
+        LibTerminus.enforceIsController();
        LibTerminus.TerminusStorage storage ts = LibTerminus.terminusStorage();
        uint256 requiredPayment = ts.poolBasePrice;
        IERC20 paymentTokenContract = _paymentTokenContract();
@ -193,6 +199,7 @@ contract TerminusFacet is ERC1155WithTerminusStorage {
        bool _transferable,
        bool _burnable
    ) external returns (uint256) {
+        LibTerminus.enforceIsController();
        LibTerminus.TerminusStorage storage ts = LibTerminus.terminusStorage();
        // TODO(zomglings): Implement requiredPayment update based on pool features.
        uint256 requiredPayment = ts.poolBasePrice;
--- a/dao/TerminusFacet.py
+++ b/dao/TerminusFacet.py
@ -217,6 +217,12 @@ class TerminusFacet:
        self.assert_contract_is_instantiated()
        return self.contract.setContractURI(_contract_uri, transaction_config)

+    def set_controller(
+        self, new_controller: ChecksumAddress, transaction_config
+    ) -> Any:
+        self.assert_contract_is_instantiated()
+        return self.contract.setController(new_controller, transaction_config)
+
    def set_payment_token(
        self, new_payment_token: ChecksumAddress, transaction_config
    ) -> Any:
@ -536,6 +542,16 @@ def handle_set_contract_uri(args: argparse.Namespace) -> None:
    print(result)


+def handle_set_controller(args: argparse.Namespace) -> None:
+    network.connect(args.network)
+    contract = TerminusFacet(args.address)
+    transaction_config = get_transaction_config(args)
+    result = contract.set_controller(
+        new_controller=args.new_controller, transaction_config=transaction_config
+    )
+    print(result)
+
+
 def handle_set_payment_token(args: argparse.Namespace) -> None:
    network.connect(args.network)
    contract = TerminusFacet(args.address)
@ -834,6 +850,13 @@ def generate_cli() -> argparse.ArgumentParser:
    )
    set_contract_uri_parser.set_defaults(func=handle_set_contract_uri)

+    set_controller_parser = subcommands.add_parser("set-controller")
+    add_default_arguments(set_controller_parser, True)
+    set_controller_parser.add_argument(
+        "--new-controller", required=True, help="Type: address"
+    )
+    set_controller_parser.set_defaults(func=handle_set_controller)
+
    set_payment_token_parser = subcommands.add_parser("set-payment-token")
    add_default_arguments(set_payment_token_parser, True)
    set_payment_token_parser.add_argument(
--- a/dao/test_terminus.py
+++ b/dao/test_terminus.py
@ -2,6 +2,7 @@ from typing import List
 import unittest

 from brownie import accounts
+from brownie.exceptions import VirtualMachineError

 from . import ERC20Facet, TerminusFacet, TerminusInitializer
 from .core import facet_cut
@ -32,6 +33,34 @@ class TestDeployment(MoonstreamDAOSingleContractTestCase):
        self.assertEqual(controller, accounts[0].address)


+class TestController(TerminusTestCase):
+    def test_set_controller_fails_when_not_called_by_controller(self):
+        terminus_diamond_address = self.terminus_contracts["Diamond"]
+        diamond_terminus = TerminusFacet.TerminusFacet(terminus_diamond_address)
+
+        with self.assertRaises(VirtualMachineError):
+            diamond_terminus.set_controller(accounts[1].address, {"from": accounts[1]})
+
+    def test_set_controller_fails_when_not_called_by_controller_even_if_they_change_to_existing_controller(
+        self,
+    ):
+        terminus_diamond_address = self.terminus_contracts["Diamond"]
+        diamond_terminus = TerminusFacet.TerminusFacet(terminus_diamond_address)
+
+        with self.assertRaises(VirtualMachineError):
+            diamond_terminus.set_controller(accounts[0].address, {"from": accounts[1]})
+
+    def test_set_controller(self):
+        terminus_diamond_address = self.terminus_contracts["Diamond"]
+        diamond_terminus = TerminusFacet.TerminusFacet(terminus_diamond_address)
+
+        self.assertEqual(diamond_terminus.terminus_controller(), accounts[0].address)
+        diamond_terminus.set_controller(accounts[3].address, {"from": accounts[0]})
+        self.assertEqual(diamond_terminus.terminus_controller(), accounts[3].address)
+        diamond_terminus.set_controller(accounts[0].address, {"from": accounts[3]})
+        self.assertEqual(diamond_terminus.terminus_controller(), accounts[0].address)
+
+
 class TestContractURI(TerminusTestCase):
    def test_contract_uri(self):
        terminus_diamond_address = self.terminus_contracts["Diamond"]
@ -64,12 +93,14 @@ class TestPoolCreation(TerminusTestCase):
        pool_base_price = diamond_terminus.pool_base_price()
        self.assertEqual(pool_base_price, 1000)

+        diamond_terminus.set_controller(accounts[1].address, {"from": accounts[0]})
+
        diamond_moonstream.mint(accounts[1], 1000, {"from": accounts[0]})
        initial_payer_balance = diamond_moonstream.balance_of(accounts[1].address)
        initial_terminus_balance = diamond_moonstream.balance_of(
            terminus_diamond_address
        )
-        initial_controller_balance = diamond_moonstream.balance_of(accounts[0].address)
+        initial_controller_balance = diamond_moonstream.balance_of(accounts[1].address)

        diamond_moonstream.approve(
            terminus_diamond_address, 1000, {"from": accounts[1]}
@ -87,20 +118,17 @@ class TestPoolCreation(TerminusTestCase):
            terminus_diamond_address
        )
        intermediate_controller_balance = diamond_moonstream.balance_of(
-            accounts[0].address
+            accounts[1].address
        )
        self.assertEqual(final_payer_balance, initial_payer_balance - 1000)
        self.assertEqual(intermediate_terminus_balance, initial_terminus_balance + 1000)
-        self.assertEqual(intermediate_controller_balance, initial_controller_balance)
+        self.assertEqual(
+            intermediate_controller_balance, initial_controller_balance - 1000
+        )

        with self.assertRaises(Exception):
            diamond_terminus.withdraw_payments(
-                accounts[1].address, 1000, {"from": accounts[1]}
-            )
-
-        with self.assertRaises(Exception):
-            diamond_terminus.withdraw_payments(
-                accounts[0].address, 1000, {"from": accounts[1]}
+                accounts[0].address, 1000, {"from": accounts[0]}
            )

        with self.assertRaises(Exception):
@ -108,12 +136,17 @@ class TestPoolCreation(TerminusTestCase):
                accounts[1].address, 1000, {"from": accounts[0]}
            )

+        with self.assertRaises(Exception):
+            diamond_terminus.withdraw_payments(
+                accounts[0].address, 1000, {"from": accounts[1]}
+            )
+
        diamond_terminus.withdraw_payments(
-            accounts[0].address, 1000, {"from": accounts[0]}
+            accounts[1].address, 1000, {"from": accounts[1]}
        )

        final_terminus_balance = diamond_moonstream.balance_of(terminus_diamond_address)
-        final_controller_balance = diamond_moonstream.balance_of(accounts[0].address)
+        final_controller_balance = diamond_moonstream.balance_of(accounts[1].address)
        self.assertEqual(final_terminus_balance, intermediate_terminus_balance - 1000)
        self.assertEqual(
            final_controller_balance, intermediate_controller_balance + 1000
@ -152,6 +185,8 @@ class TestPoolOperations(TerminusTestCase):
        cls.diamond_terminus = diamond_terminus
        cls.diamond_moonstream = diamond_moonstream

+        cls.diamond_terminus.set_controller(accounts[1].address, {"from": accounts[0]})
+
    def setUp(self) -> None:
        self.diamond_terminus.create_simple_pool(10, {"from": accounts[1]})

--- a/operations/customer-f3bc8a15-3b10-4be5-817e-0f16b4a31b6a/terminus-update-mainnet-20220201-1442.md
+++ b/operations/customer-f3bc8a15-3b10-4be5-817e-0f16b4a31b6a/terminus-update-mainnet-20220201-1442.md
@ -0,0 +1,76 @@
+# Update the Terminus contract
+
+The Terminus contract is deployed as an EIP2535 Diamond proxy contract with a Terminus facet attached to it.
+
+This checklist describes how to update the `TerminusFacet` on the Terminus diamond contract.
+
+## Deployed addresses
+
+You will modify this section as you go through the checklist
+
+### `TerminusFacet` address
+
+```
+export TERMINUS_FACET_ADDRESS="0x6396813307826Fb315e65CA7138A41CFa09a8AB3"
+```
+
+## Environment variables
+
+- [x] `export DAO_NETWORK=<desired brownie network>`
+- [x] `export DAO_OWNER=<path to keystore file for owner account>`
+- [x] `export DAO_OWNER_ADDRESS=$(jq -r .address $DAO_OWNER)`
+- [x] `export MAX_FEE_PER_GAS="200 gwei"`
+- [x] `export MAX_PRIORITY_FEE_PER_GAS="80 gwei"`
+- [x] `export CONFIRMATIONS=1`
+- [x] `export TERMINUS_DIAMOND=0x99A558BDBdE247C2B2716f0D4cFb0E246DFB697D`
+
+## Detach existing `TerminusFacet`
+
+- [x] Remove `TerminusFacet` from diamond. (This may require checkout of earlier commit and `brownie compile`.)
+
+```bash
+dao core facet-cut \
+    --address $TERMINUS_DIAMOND \
+    --network $DAO_NETWORK \
+    --sender $DAO_OWNER \
+    --max-fee-per-gas "$MAX_FEE_PER_GAS" \
+    --max-priority-fee-per-gas "$MAX_PRIORITY_FEE_PER_GAS" \
+    --confirmations $CONFIRMATIONS \
+    --facet-name TerminusFacet \
+    --action remove
+```
+
+
+## Deploy `TerminusFacet`
+
+- [x] Check out relevant commit and `brownie compile`.
+
+- [x] Deploy `TerminusFacet` contract
+
+```bash
+dao terminus deploy \
+    --network $DAO_NETWORK \
+    --sender $DAO_OWNER \
+    --max-fee-per-gas "$MAX_FEE_PER_GAS" \
+    --max-priority-fee-per-gas "$MAX_PRIORITY_FEE_PER_GAS" \
+    --confirmations $CONFIRMATIONS
+```
+
+- [x] Export address of deployed contract as `export TERMINUS_FACET_ADDRESS=0x6396813307826Fb315e65CA7138A41CFa09a8AB3`
+
+- [x] Store address of deployed contract under `Deployed addresses / TerminusFacet address` above
+
+- [x] Attach `TerminusFacet` to diamond:
+
+```bash
+dao core facet-cut \
+    --address $TERMINUS_DIAMOND \
+    --network $DAO_NETWORK \
+    --sender $DAO_OWNER \
+    --max-fee-per-gas "$MAX_FEE_PER_GAS" \
+    --max-priority-fee-per-gas "$MAX_PRIORITY_FEE_PER_GAS" \
+    --confirmations $CONFIRMATIONS \
+    --facet-name TerminusFacet \
+    --facet-address $TERMINUS_FACET_ADDRESS \
+    --action add
+```
--- a/operations/terminus-update-mainnet-20220201-1442.md
+++ b/operations/terminus-update-mainnet-20220201-1442.md
@ -0,0 +1,66 @@
+# Update the Terminus contract
+
+The Terminus contract is deployed as an EIP2535 Diamond proxy contract with a Terminus facet attached to it.
+
+This checklist describes how to update the `TerminusFacet` on the Terminus diamond contract.
+
+## Deployed addresses
+
+You will modify this section as you go through the checklist
+
+### `TerminusFacet` address
+
+```
+export TERMINUS_FACET_ADDRESS="0x6396813307826Fb315e65CA7138A41CFa09a8AB3"
+```
+
+## Environment variables
+
+- [x] `export DAO_NETWORK=<desired brownie network>`
+- [x] `export DAO_OWNER=<path to keystore file for owner account>`
+- [x] `export DAO_OWNER_ADDRESS=$(jq -r .address $DAO_OWNER)`
+- [x] `export MAX_FEE_PER_GAS="200 gwei"`
+- [x] `export MAX_PRIORITY_FEE_PER_GAS="80 gwei"`
+- [x] `export CONFIRMATIONS=1`
+- [x] `export TERMINUS_DIAMOND=0x062BEc5e84289Da2CD6147E0e4DA402B33B8f796`
+
+
+## Detach existing `TerminusFacet`
+
+- [x] Remove `TerminusFacet` from diamond. (This may require checkout of earlier commit and `brownie compile`.)
+
+```bash
+dao core facet-cut \
+    --address $TERMINUS_DIAMOND \
+    --network $DAO_NETWORK \
+    --sender $DAO_OWNER \
+    --max-fee-per-gas "$MAX_FEE_PER_GAS" \
+    --max-priority-fee-per-gas "$MAX_PRIORITY_FEE_PER_GAS" \
+    --confirmations $CONFIRMATIONS \
+    --facet-name TerminusFacet \
+    --action remove
+```
+
+
+## Deploy `TerminusFacet`
+
+- [x] Check out correct branch and `brownie compile`.
+
+- [x] Export address of deployed contract as `export TERMINUS_FACET_ADDRESS=0x6396813307826Fb315e65CA7138A41CFa09a8AB3`
+
+- [x] Store address of deployed contract under `Deployed addresses / TerminusFacet address` above
+
+- [ ] Attach `TerminusFacet` to diamond:
+
+```bash
+dao core facet-cut \
+    --address $TERMINUS_DIAMOND \
+    --network $DAO_NETWORK \
+    --sender $DAO_OWNER \
+    --max-fee-per-gas "$MAX_FEE_PER_GAS" \
+    --max-priority-fee-per-gas "$MAX_PRIORITY_FEE_PER_GAS" \
+    --confirmations $CONFIRMATIONS \
+    --facet-name TerminusFacet \
+    --facet-address $TERMINUS_FACET_ADDRESS \
+    --action add
+```