From 346cd4f48e8926346f81861ef52a51f88b74053a Mon Sep 17 00:00:00 2001
From: David Terry <me@xwvvvvwx.com>
Date: Sun, 28 Jul 2019 17:24:56 +0200
Subject: [PATCH] remove proxy

This only protected against one edge case attack (a malicous plan that
plotted new plans that could be immediately executed).

Given that plot is authed and a malcious plan could anyway take
ownership of the entire pause, this counter-measure is not worth the
addtional complexity.
---
 readme.md             | 22 +++++++---------------
 src/integration.t.sol |  4 ++--
 src/pause.sol         | 22 ++--------------------
 src/pause.t.sol       | 11 -----------
 4 files changed, 11 insertions(+), 48 deletions(-)

diff --git a/readme.md b/readme.md
index 845d283..d245165 100644
--- a/readme.md
+++ b/readme.md
@@ -41,11 +41,6 @@ Plans can be manipulated in the following ways:
 A break of any of the following would be classified as a critical issue. Please submit bug reports
 to security@dapp.org.
 
-**high level**
-- There is no way to bypass the delay
-- The code executed by the `delegatecall` cannot directly modify storage on the pause
-- The pause will always retain ownership of it's `proxy`
-
 **admin**
 - `authority`, `owner`, and `delay` can only be changed if an authorized user plots a `plan` to do so
 
@@ -63,15 +58,13 @@ to security@dapp.org.
 **`drop`**
 - A `plan` can only be dropped by authorized users
 
-## Identity & Trust
-
-In order to protect the internal storage of the pause from malicious writes during `plan` execution,
-we perform the actual `delegatecall` operation in a seperate contract with an isolated storage
-context (`DSPauseProxy`). Each pause has it's own individual `proxy`.
+## Gotchas
 
-This means that `plan`'s are executed with the identity of the `proxy`, and when integrating the
-pause into some auth scheme, you probably want to trust the pause's `proxy` and not the pause
-itself.
+Plans are executed with `delegatecall`, this means they have full write access to the pause's
+storage. This access can be used to bypass the checks in `plot` and make plans that can be executed
+less than `delay` seconds into the future. Because `plot` is only callable by authorized users, and
+a malicous plan could anyway just take ownership of the pause entirely, it was decided that counter
+measures were not worth the additional complexity.
 
 ## Example Usage
 
@@ -95,8 +88,7 @@ pause.plot(usr, tag, fax, eta);
 ```
 
 ```solidity
-// wait until block.timestamp is at least now + delay...
-// and then execute the plan
+// wait until block.timestamp is at least now + delay and then execute the plan
 
 bytes memory out = pause.exec(usr, tag, fax, eta);
 ```
diff --git a/src/integration.t.sol b/src/integration.t.sol
index e2fa199..0dc2933 100644
--- a/src/integration.t.sol
+++ b/src/integration.t.sol
@@ -165,7 +165,7 @@ contract Voting is Test {
         // create gov system
         DSChief chief = chiefFab.newChief(gov, maxSlateSize);
         DSPause pause = new DSPause(delay, address(0x0), chief);
-        target.rely(address(pause.proxy()));
+        target.rely(address(pause));
         target.deny(address(this));
 
         // create proposal
@@ -272,7 +272,7 @@ contract UpgradeChief is Test {
         DSPause pause = new DSPause(delay, address(0x0), oldChief);
 
         // make pause the only owner of the target
-        target.rely(address(pause.proxy()));
+        target.rely(address(pause));
         target.deny(address(this));
 
         // create new chief
diff --git a/src/pause.sol b/src/pause.sol
index c49880f..1beb28b 100644
--- a/src/pause.sol
+++ b/src/pause.sol
@@ -22,7 +22,7 @@ contract DSPause is DSAuth, DSNote {
 
     // --- admin ---
 
-    modifier wait { require(msg.sender == address(proxy), "ds-pause-undelayed-call"); _; }
+    modifier wait { require(msg.sender == address(this), "ds-pause-undelayed-call"); _; }
 
     function setOwner(address owner_) public wait {
         owner = owner_;
@@ -45,9 +45,8 @@ contract DSPause is DSAuth, DSNote {
 
     // --- data ---
 
+    uint public delay;
     mapping (bytes32 => bool) public plans;
-    DSPauseProxy public proxy;
-    uint         public delay;
 
     // --- init ---
 
@@ -55,7 +54,6 @@ contract DSPause is DSAuth, DSNote {
         delay = delay_;
         owner = owner_;
         authority = authority_;
-        proxy = new DSPauseProxy();
     }
 
     // --- util ---
@@ -99,22 +97,6 @@ contract DSPause is DSAuth, DSNote {
 
         plans[hash(usr, tag, fax, eta)] = false;
 
-        out = proxy.exec(usr, fax);
-        require(proxy.owner() == address(this), "ds-pause-illegal-storage-change");
-    }
-}
-
-// plans are executed in an isolated storage context to protect the pause from
-// malicious storage modification during plan execution
-contract DSPauseProxy {
-    address public owner;
-    modifier auth { require(msg.sender == owner, "ds-pause-proxy-unauthorized"); _; }
-    constructor() public { owner = msg.sender; }
-
-    function exec(address usr, bytes memory fax)
-        public auth
-        returns (bytes memory out)
-    {
         bool ok;
         (ok, out) = usr.delegatecall(fax);
         require(ok, "ds-pause-delegatecall-error");
diff --git a/src/pause.t.sol b/src/pause.t.sol
index 939fa71..95b1329 100644
--- a/src/pause.t.sol
+++ b/src/pause.t.sol
@@ -276,17 +276,6 @@ contract Exec is Test {
         pause.exec(usr, tag, fax, eta);
     }
 
-    function testFail_exec_plan_with_proxy_ownership_change() public {
-        address      usr = target;
-        bytes32      tag = extcodehash(usr);
-        bytes memory fax = abi.encodeWithSignature("give(address)", address(this));
-        uint         eta = now + pause.delay();
-
-        pause.plot(usr, tag, fax, eta);
-        hevm.warp(eta);
-        pause.exec(usr, tag, fax, eta);
-    }
-
     function test_suceeds_when_delay_passed() public {
         address      usr = target;
         bytes32      tag = extcodehash(usr);