refactor: remove methods relying on conforming spells

makerdao · May 15, 2024 · a3cd851 · a3cd851
1 parent 5d05b7f
commit a3cd851
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 160 deletions.
diff --git a/README.md b/README.md
@@ -9,60 +9,51 @@ permissionlessly drop any scheduled plan.
 - A `plan` is a scheduled `delegatecall` that exists in [`MCD_PAUSE`](https://github.com/makerdao/ds-pause) and can be
   permissionlessly executed.
 - Plans in `MCD_PAUSE` are [identified by a unique 32 byte hash](https://github.com/makerdao/ds-pause/blob/master/src/pause.sol#L99).
-- A conformant spell here is one where the values returned by `action()`, `tag()`, `sig()` and `eta()` are equal to the
-  corresponding `plan`'s `usr`, `tag`, `fax` and `eta` values respectively. Bad actors could potentially manipulate the
-  return values of these functions such that they are not equal, which would result in a non-conformant spell.
+- A conforming spell here is one where the values returned by `action()`, `tag()`, `sig()` and `eta()` are equal to the
+  corresponding `plan`, `usr`, `tag`, `fax` and `eta` values respectively. Bad actors could potentially manipulate the
+  return values of these functions such that they are not equal, which would result in a non-conforming spell.
+- Users interacting with `Protego` should **always** assume the spell they want to drop is non-conforming and fetch the
+    parameters directly from the logs created by [`pause.plot()`](https://etherscan.deth.net/address/0xbe286431454714f511008713973d3b053a2d38f3#L189-L203)
 
 ## Usage
 
 ### Create a single drop spell
 
-If there is sufficient ecosystem interest to cancel (`drop`) a particular scheduled set of actions (the target `plan`),
-Protego contains a factory to permissionlessly create a spell ("drop spell"), which &ndash; upon being given the hat
-&ndash; is able to drop the targeted plan in `MCD_PAUSE`.
-
-There are two possibilities:
-
-#### 1. `deploy(DssSpellLike _spell)(address)`
+```solidity
+deploy(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(address)
+```
 
-Used for [conformant spells](https://github.com/makerdao/spells-mainnet), will create a drop spell targeting a plan in
-`MCD_PAUSE`, using the values that the spell provides.
-
-#### 2. `deploy(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(address)`
-
-Used for non-conformat spells, will create a drop spell targeting a plan in `MCD_PAUSE` using values provided manually
-&ndash; which can be found in the payload of the transaction that originally planned it.
+If there is sufficient ecosystem interest to cancel (`drop`) a particular scheduled set of actions (the target `plan`),
+Protego contains a factory to permissionlessly create a spell ("Emergency Drop Spell"), which &ndash; upon being given
+the hat &ndash; is able to drop the targeted plan in `MCD_PAUSE`.
 
 ### Enable permissionless dropping of any plan
 
+```solidity
+drop(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)
+```
+
 In case of a governance attack, numerous spells could be created and planned by an attacker at a rate faster than a
 single hat spell can `drop` them. To mitigate this risk, the `Protego` contract itself can be given the hat, which
 allows any user to permissionlessly drop any plan.
 
-Once again, there are two alternatives:
-
-#### 1. `drop(DssSpellLike _spell)`
-
-Used for conformat spells, will **immediately** drop a plan in `MCD_PAUSE` using the values the spell provides.
-
-#### 2. `drop(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)`
-
-Used for non-conformant spells, will **immediately** drop a plan in `MCD_PAUSE` using values provided manually &ndash;
-which can be found in the payload of the transaction that originally planned it.
+Used for conforming spells, will **immediately** drop a plan in `MCD_PAUSE` using the values the spell provides.
 
 ### Additional functions
 
 #### `id()`
 
-- `id(DssSpellLike _spell)(bytes32)`
-- `id(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(bytes32)`
+```solidity
+id(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(bytes32)
+```
 
 Returns the `bytes32` id of a given plan using the same method that `hash` does in `MCD_PAUSE`.
 
 #### `planned()`
 
-- `planned(DssSpellLike _spell)(bool)`
-- `planned(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(bool)`
-- `planned(bytes32 _id)(bool)`
+```solidity
+planned(address _usr, bytes32 _tag, bytes memory _fax, uint256 _eta)(bool)
+planned(bytes32 _id)(bool)
+```
 
 Returns `true` if a plan has been scheduled for execution.
diff --git a/src/Protego.sol b/src/Protego.sol
@@ -47,13 +47,6 @@ contract Protego {
         pause = _pause;
     }
 
-    /// @notice Deploys a spell to drop a conformant `DssSpell`
-    /// @param _spell The spell address.
-    /// @return The `EmergencyDropSpell` address.
-    function deploy(DsSpellLike _spell) external returns (address) {
-        return deploy(_spell.action(), _spell.tag(), _spell.sig(), _spell.eta());
-    }
-
     /// @notice Deploys a spell to drop a plan based on attributes.
     /// @param _usr The address lifted to the hat.
     /// @param _tag The tag identifying the address.
@@ -74,12 +67,6 @@ contract Protego {
         return keccak256(abi.encode(_usr, _tag, _fax, _eta));
     }
 
-    /// @notice Calculates the id of a conformant `DssSpell`.
-    /// @param _spell The spell address.
-    function id(DsSpellLike _spell) public view returns (bytes32) {
-        return id(_spell.action(), _spell.tag(), _spell.sig(), _spell.eta());
-    }
-
     /// @notice Returns whether a plan matching the set of attributes is currently planned.
     /// @param _usr The address lifted to the hat.
     /// @param _tag The tag identifying the address.
@@ -89,23 +76,17 @@ contract Protego {
         return planned(id(_usr, _tag, _fax, _eta));
     }
 
-    /// @notice Returns whether aspell is planned or not.
-    /// @param _spell The spell address.
-    function planned(DsSpellLike _spell) external view returns (bool) {
-        return planned(id(_spell));
-    }
-
     /// @notice Returns whether an id is planned or not.
     /// @param _id The ide of the plan.
     function planned(bytes32 _id) public view returns (bool) {
         return DsPauseLike(pause).plans(_id);
     }
 
-    /// @notice Permissionlessly drop anything.
-    /// @dev In some cases, due to a governance attack or other unforseen causes, it may be necessary to block any spell
-    ///      that is entered into the pause proxy.
-    ///      In this extreme case, the system can be protected during the pause delay by lifting the Protego contract to
-    ///      the hat role, which will allow any user to permissionlessly drop any id from the pause.
+    /// @notice Permissionlessly drop anything that has been planned on the pause.
+    /// @dev In some cases, due to a faulty spell being voted, a governance attack or other unforseen causes, it may be
+    ///      necessary to block any spell that is entered into the pause proxy.
+    ///      In this extreme case, the system can be protected during the pause delay by lifting the Protego contract
+    ///      to the hat role, which will allow any user to permissionlessly drop any id from the pause.
     ///      This function is expected to revert if it does not have the authority to perform this function.
     /// @param _usr The address lifted to the hat.
     /// @param _tag The tag identifying the address.
@@ -115,15 +96,4 @@ contract Protego {
         DsPauseLike(pause).drop(_usr, _tag, _fax, _eta);
         emit Drop(id(_usr, _tag, _fax, _eta));
     }
-
-    /// @notice Permissionlessly drop a conformat spell.
-    /// @dev In some cases, due to a governance attack or other unforseen causes, it may be necessary to block any spell
-    ///      that is entered into the pause proxy.
-    ///      In this extreme case, the system can be protected during the pause delay by lifting the Protego contract to
-    ///      the hat role, which will allow any user to permissionlessly drop any id from the pause.
-    ///      This function is expected to revert if it does not have the authority to perform this function.
-    /// @param _spell The address of the spell lifted to the hat.
-    function drop(DsSpellLike _spell) external {
-        drop(_spell.action(), _spell.tag(), _spell.sig(), _spell.eta());
-    }
 }
diff --git a/src/Protego.t.sol b/src/Protego.t.sol
@@ -80,58 +80,39 @@ contract ProtegoTest is DssTest {
     function testDeploySpell() public {
         ConformingSpell badSpell = new ConformingSpell(pause, end);
 
-        assertFalse(protego.planned(DsSpellLike(address(badSpell))));
-
-        address usr = badSpell.action();
-        bytes32 tag = badSpell.tag();
-        bytes memory sig = badSpell.sig();
-        uint256 eta = badSpell.eta();
+        assertFalse(
+            protego.planned(badSpell.action(), badSpell.tag(), badSpell.sig(), badSpell.eta()),
+            "Spell already planned for params"
+        );
 
         vm.expectEmit(true, true, true, true);
         emit Deploy(vm.computeCreateAddress(address(protego), vm.getNonce(address(protego))));
-        address goodSpell = protego.deploy(DsSpellLike(address(badSpell)));
-
-        assertEq(protego.id(DsSpellLike(goodSpell)), protego.id(usr, tag, sig, eta));
-    }
-
-    function testDeploySpellParams() public {
-        ConformingSpell badSpell = new ConformingSpell(pause, end);
-
-        assertFalse(protego.planned(DsSpellLike(address(badSpell))));
-
-        address usr = badSpell.action();
-        bytes32 tag = badSpell.tag();
-        bytes memory sig = badSpell.sig();
-        uint256 eta = badSpell.eta();
-
-        vm.expectEmit(true, true, true, true);
-        emit Deploy(vm.computeCreateAddress(address(protego), vm.getNonce(address(protego))));
-        address goodSpell = protego.deploy(usr, tag, sig, eta);
-
-        assertEq(protego.id(DsSpellLike(goodSpell)), protego.id(usr, tag, sig, eta));
+        protego.deploy(badSpell.action(), badSpell.tag(), badSpell.sig(), badSpell.eta());
     }
 
     function testPlanned() public {
         ConformingSpell badSpell = new ConformingSpell(pause, end);
 
-        assertFalse(protego.planned(DsSpellLike(address(badSpell))), "Spell already planned");
+        assertFalse(
+            protego.planned(badSpell.action(), badSpell.tag(), badSpell.sig(), badSpell.eta()),
+            "Spell already planned for params"
+        );
 
         _vote(address(badSpell));
         badSpell.schedule();
 
-        address usr = badSpell.action();
-        bytes32 tag = badSpell.tag();
-        bytes memory sig = badSpell.sig();
-        uint256 eta = badSpell.eta();
-
-        assertTrue(protego.planned(usr, tag, sig, eta), "Planned failed for params");
-        assertTrue(protego.planned(DsSpellLike(address(badSpell))), "Planned failed for address");
-        assertTrue(protego.planned(protego.id(DsSpellLike(address(badSpell)))), "Planned failed for id");
+        assertTrue(
+            protego.planned(badSpell.action(), badSpell.tag(), badSpell.sig(), badSpell.eta()),
+            "Planned failed for params"
+        );
+        assertTrue(
+            protego.planned(protego.id(badSpell.action(), badSpell.tag(), badSpell.sig(), badSpell.eta())),
+            "Planned failed for id"
+        );
     }
 
     function testId() public {
         ConformingSpell badSpell = new ConformingSpell(pause, end);
-
         address usr = badSpell.action();
         bytes32 tag = badSpell.tag();
         bytes memory sig = badSpell.sig();
@@ -140,31 +121,6 @@ contract ProtegoTest is DssTest {
         bytes32 id = keccak256(abi.encode(usr, tag, sig, eta));
 
         assertEq(id, protego.id(usr, tag, sig, eta), "Invalid id from params");
-        assertEq(id, protego.id(DsSpellLike(address(badSpell))), "Invalid id from spell");
-    }
-
-    // Test drop of conformant spell
-    function testDropSpell() public {
-        ConformingSpell badSpell = new ConformingSpell(pause, end);
-
-        assertFalse(protego.planned(DsSpellLike(address(badSpell))), "Spell already planned");
-
-        _vote(address(badSpell));
-        badSpell.schedule();
-
-        address usr = badSpell.action();
-        bytes32 tag = badSpell.tag();
-        bytes memory sig = badSpell.sig();
-        uint256 eta = badSpell.eta();
-
-        assertTrue(protego.planned(usr, tag, sig, eta));
-
-        EmergencyDropSpell goodSpell = EmergencyDropSpell(protego.deploy(DsSpellLike(address(badSpell))));
-        _vote(address(goodSpell));
-        goodSpell.schedule();
-
-        assertFalse(protego.planned(usr, tag, sig, eta), "After drop spell: spell still planned (params)");
-        assertFalse(protego.planned(DsSpellLike(address(badSpell))), "After drop spell: spell still planned (address)");
     }
 
     // Test drop of spell created with params
@@ -190,37 +146,6 @@ contract ProtegoTest is DssTest {
         assertFalse(protego.planned(usr, tag, sig, eta), "After drop spell: spell still planned");
     }
 
-    // Test drop anything by lifting Protego to hat
-    function testDropAllSpells() public {
-        uint256 iter = 10;
-        BadSpells[] memory badSpells = new BadSpells[](iter);
-
-        for (uint256 i = 0; i < iter; i++) {
-            ConformingSpell badSpell = new ConformingSpell(pause, end);
-            _vote(address(badSpell));
-            badSpell.schedule();
-
-            badSpells[i].badSpell = address(badSpell);
-
-            assertTrue(protego.planned(DsSpellLike(address(badSpell))));
-        }
-
-        _vote(address(protego));
-
-        for (uint256 i = 0; i < iter; i++) {
-            vm.expectEmit(true, true, true, true);
-            emit Drop(protego.id(DsSpellLike(badSpells[i].badSpell)));
-            protego.drop(DsSpellLike(badSpells[i].badSpell));
-
-            assertFalse(protego.planned(DsSpellLike(badSpells[i].badSpell)));
-        }
-
-        // After Protego loses the hat, it can no longer drop spells
-        _vote(address(0));
-        vm.expectRevert();
-        protego.drop(DsSpellLike(badSpells[0].badSpell));
-    }
-
     // Test drop anything by lifting Protego to hat
     function testDropAllSpellsParams() public {
         uint256 iter = 10;
@@ -252,7 +177,7 @@ contract ProtegoTest is DssTest {
         // After Protego loses the hat, it can no longer drop spells
         _vote(address(0));
         vm.expectRevert();
-        protego.drop(DsSpellLike(badSpells[0].badSpell));
+        protego.drop(badSpells[0].action, badSpells[0].tag, badSpells[0].sig, badSpells[0].eta);
     }
 
     function _vote(address spell_) internal {