Merge pull request #114 from PaulRBerg/refactor/remove-min-gas-reserve

refactor: remove "minGasReserve"
PaulRBerg · Jun 17, 2023 · 2ebcbc6 · 2ebcbc6
2 parents afa7f7f + 559819c
commit 2ebcbc6
Show file tree

Hide file tree

Showing 17 changed files with 57 additions and 225 deletions.
diff --git a/README.md b/README.md
@@ -95,7 +95,6 @@ Once the proxy is deployed, you can start interacting with target contracts. PRB
 `PRBProxyAnnex`. This contract provides several auxiliary functions, including:
 
 - `installPlugin`
-- `setMinGasReserve`
 - `setPermission`
 - `uninstallPlugin`
 

diff --git a/src/PRBProxy.sol b/src/PRBProxy.sol
@@ -37,7 +37,6 @@ contract PRBProxy is
     /// @notice Constructs the proxy by fetching the params from the registry.
     /// @dev This is implemented like this so that the proxy's CREATE2 address doesn't depend on the constructor params.
     constructor() {
-        minGasReserve = 5000;
         registry = IPRBProxyRegistry(msg.sender);
         owner = registry.transientProxyOwner();
     }
@@ -138,11 +137,8 @@ contract PRBProxy is
         // Save the owner address in memory so that this variable cannot be modified during the DELEGATECALL.
         address owner_ = owner;
 
-        // Reserve some gas to ensure that the contract call will not run out of gas.
-        uint256 stipend = gasleft() - minGasReserve;
-
         // Delegate call to the provided contract.
-        (success, response) = to.delegatecall{ gas: stipend }(data);
+        (success, response) = to.delegatecall(data);
 
         // Check that the owner has not been changed.
         if (owner_ != owner) {

diff --git a/src/PRBProxyAnnex.sol b/src/PRBProxyAnnex.sol
@@ -63,18 +63,6 @@ contract PRBProxyAnnex is
         emit InstallPlugin(plugin);
     }
 
-    /// @inheritdoc IPRBProxyAnnex
-    function setMinGasReserve(uint256 newMinGasReserve) external override {
-        // Load the current minimum gas reserve.
-        uint256 oldMinGasReserve = minGasReserve;
-
-        // Update the minimum gas reserve.
-        minGasReserve = newMinGasReserve;
-
-        // Log the minimum gas reserve update.
-        emit SetMinGasReserve(oldMinGasReserve, newMinGasReserve);
-    }
-
     /// @inheritdoc IPRBProxyAnnex
     function setPermission(address envoy, address target, bool permission) external override {
         permissions[envoy][target] = permission;

diff --git a/src/abstracts/PRBProxyStorage.sol b/src/abstracts/PRBProxyStorage.sol
@@ -10,9 +10,6 @@ abstract contract PRBProxyStorage is IPRBProxyStorage {
     /// @inheritdoc IPRBProxyStorage
     address public override owner;
 
-    /// @inheritdoc IPRBProxyStorage
-    uint256 public override minGasReserve;
-
     /// @inheritdoc IPRBProxyStorage
     mapping(bytes4 method => IPRBProxyPlugin plugin) public plugins;
 

diff --git a/src/interfaces/IPRBProxy.sol b/src/interfaces/IPRBProxy.sol
@@ -62,7 +62,6 @@ interface IPRBProxy is IPRBProxyStorage {
     /// Requirements:
     /// - The caller must be either an owner or an envoy with permission.
     /// - `target` must be a contract.
-    /// - The gas stipend must be greater than or equal to `minGasReserve`.
     /// - The owner must not be changed during the DELEGATECALL.
     ///
     /// @param target The address of the target contract.

diff --git a/src/interfaces/IPRBProxyAnnex.sol b/src/interfaces/IPRBProxyAnnex.sol
@@ -24,9 +24,6 @@ interface IPRBProxyAnnex {
     /// @notice Emitted when a plugin is installed.
     event InstallPlugin(IPRBProxyPlugin indexed plugin);
 
-    /// @notice Emitted when the minimum gas reserve is updated.
-    event SetMinGasReserve(uint256 oldMinGasReserve, uint256 newMinGasReserve);
-
     /// @notice Emitted when the permission is updated for an (envoy,target) tuple.
     event SetPermission(address indexed envoy, address indexed target, bool permission);
 
@@ -60,13 +57,6 @@ interface IPRBProxyAnnex {
     /// @param plugin The address of the plugin to install.
     function installPlugin(IPRBProxyPlugin plugin) external;
 
-    /// @notice Sets a new value for the minimum gas reserve.
-    ///
-    /// @dev Emits a {SetMinGasReserve} event.
-    ///
-    /// @param newMinGasReserve The new minimum gas reserve.
-    function setMinGasReserve(uint256 newMinGasReserve) external;
-
     /// @notice Gives or takes a permission from an envoy to call the provided target contract and function selector
     /// on behalf of the proxy owner.
     ///
@@ -90,7 +80,6 @@ interface IPRBProxyAnnex {
     /// Requirements:
     /// - The plugin must have at least one implemented method.
     ///
-    ///
     /// @param plugin The address of the plugin to uninstall.
     function uninstallPlugin(IPRBProxyPlugin plugin) external;
 }
diff --git a/src/interfaces/IPRBProxyStorage.sol b/src/interfaces/IPRBProxyStorage.sol
@@ -9,12 +9,6 @@ interface IPRBProxyStorage {
     /// @notice The address of the owner account or contract.
     function owner() external view returns (address);
 
-    /// @notice The amount of gas to reserve for running the remainder of either the fallback or the execute
-    /// function after the delegate call.
-    /// @dev This precaution ensures that the proxy remains operational even if EVM opcode gas costs change in the
-    /// future.
-    function minGasReserve() external view returns (uint256);
-
     /// @notice The address of the plugin contract installed for the provided method.
     /// @dev The zero address is returned if no plugin contract is installed.
     /// @param method The method's signature for the query.

diff --git a/test/Base.t.sol b/test/Base.t.sol
@@ -25,7 +25,6 @@ import { TargetChangeOwner } from "./mocks/targets/TargetChangeOwner.sol";
 import { TargetDummy } from "./mocks/targets/TargetDummy.sol";
 import { TargetDummyWithFallback } from "./mocks/targets/TargetDummyWithFallback.sol";
 import { TargetEcho } from "./mocks/targets/TargetEcho.sol";
-import { TargetMinGasReserve } from "./mocks/targets/TargetMinGasReserve.sol";
 import { TargetPanic } from "./mocks/targets/TargetPanic.sol";
 import { TargetReverter } from "./mocks/targets/TargetReverter.sol";
 import { TargetSelfDestructer } from "./mocks/targets/TargetSelfDestructer.sol";
@@ -53,7 +52,6 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
         TargetDummy dummy;
         TargetDummyWithFallback dummyWithFallback;
         TargetEcho echo;
-        TargetMinGasReserve minGasReserve;
         TargetPanic panic;
         TargetReverter reverter;
         TargetSelfDestructer selfDestructer;
@@ -121,7 +119,6 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
             dummy: new TargetDummy(),
             dummyWithFallback: new TargetDummyWithFallback(),
             echo: new TargetEcho(),
-            minGasReserve: new TargetMinGasReserve(),
             panic: new TargetPanic(),
             reverter: new TargetReverter(),
             selfDestructer: new TargetSelfDestructer()
@@ -200,12 +197,6 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
         proxy.execute({ target: address(annex), data: data });
     }
 
-    /// @dev ABI encodes the parameters and calls {PRBProxyAnnex.setMinGasReserve}.
-    function setMinGasReserve(uint256 newMinGasReserve) internal {
-        bytes memory data = abi.encodeCall(annex.setMinGasReserve, (newMinGasReserve));
-        proxy.execute({ target: address(annex), data: data });
-    }
-
     /// @dev ABI encodes the parameters and calls {PRBProxyAnnex.setPermission}.
     function setPermission(address envoy, address target, bool permission) internal {
         bytes memory data = abi.encodeCall(annex.setPermission, (envoy, target, permission));

diff --git a/test/annex/set-min-gas-reserve/setMinGasReserve.t.sol b/test/annex/set-min-gas-reserve/setMinGasReserve.t.sol
diff --git a/test/annex/set-min-gas-reserve/setMinGasReserve.tree b/test/annex/set-min-gas-reserve/setMinGasReserve.tree
diff --git a/test/mocks/targets/TargetMinGasReserve.sol b/test/mocks/targets/TargetMinGasReserve.sol
diff --git a/test/proxy/execute/execute.t.sol b/test/proxy/execute/execute.t.sol
@@ -56,30 +56,7 @@ contract Execute_Test is Proxy_Test {
         _;
     }
 
-    function test_RevertWhen_GasStipendCalculationUnderflows() external whenCallerAuthorized whenTargetContract {
-        // Set the min gas reserve.
-        uint256 gasLimit = 10_000;
-        proxy.execute(
-            address(targets.minGasReserve),
-            abi.encodeWithSelector(targets.minGasReserve.setMinGasReserve.selector, gasLimit + 1)
-        );
-
-        // Run the test.
-        bytes memory data = abi.encode(targets.echo.echoUint256.selector, 1729);
-        vm.expectRevert(stdError.arithmeticError);
-        proxy.execute{ gas: gasLimit }(address(targets.echo), data);
-    }
-
-    modifier whenGasStipendCalculationDoesNotUnderflow() {
-        _;
-    }
-
-    function test_RevertWhen_OwnerChangedDuringDelegateCall()
-        external
-        whenCallerAuthorized
-        whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
-    {
+    function test_RevertWhen_OwnerChangedDuringDelegateCall() external whenCallerAuthorized whenTargetContract {
         bytes memory data = bytes.concat(targets.changeOwner.changeIt.selector);
         vm.expectRevert(abi.encodeWithSelector(IPRBProxy.PRBProxy_OwnerChanged.selector, owner, address(1729)));
         proxy.execute(address(targets.changeOwner), data);
@@ -97,7 +74,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -110,7 +86,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -123,7 +98,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -136,7 +110,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -149,7 +122,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -162,7 +134,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -175,7 +146,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -188,7 +158,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -201,7 +170,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallReverts
     {
@@ -218,7 +186,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
     {
@@ -237,7 +204,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -279,7 +245,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -296,7 +261,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -313,7 +277,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -330,7 +293,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -347,7 +309,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -364,7 +325,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -381,7 +341,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -398,7 +357,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent
@@ -415,7 +373,6 @@ contract Execute_Test is Proxy_Test {
         external
         whenCallerAuthorized
         whenTargetContract
-        whenGasStipendCalculationDoesNotUnderflow
         whenOwnerNotChangedDuringDelegateCall
         whenDelegateCallDoesNotRevert
         whenNoEtherSent