refactor: reduce base strategy size (#422)

* refactor: reduce base strategy size

* fix: do not add onlyManagement

contracts/BaseStrategy.sol

@@ -286,39 +286,30 @@ abstract contract BaseStrategy {
     bool public emergencyExit;
 
     // modifiers
-    modifier onlyAuthorized() {
-        require(msg.sender == strategist || msg.sender == governance(), "!authorized");
-        _;
+    function _onlyAuthorized() internal {
+        require(msg.sender == strategist || msg.sender == governance());
     }
 
-    modifier onlyEmergencyAuthorized() {
-        require(
-            msg.sender == strategist || msg.sender == governance() || msg.sender == vault.guardian() || msg.sender == vault.management(),
-            "!authorized"
-        );
-        _;
+    function _onlyEmergencyAuthorized() internal {
+        require(msg.sender == strategist || msg.sender == governance() || msg.sender == vault.guardian() || msg.sender == vault.management());
     }
 
-    modifier onlyStrategist() {
-        require(msg.sender == strategist, "!strategist");
-        _;
+    function _onlyStrategist() internal {
+        require(msg.sender == strategist);
     }
 
-    modifier onlyGovernance() {
-        require(msg.sender == governance(), "!authorized");
-        _;
+    function _onlyGovernance() internal {
+        require(msg.sender == governance());
     }
 
-    modifier onlyKeepers() {
+    function _onlyKeepers() internal {
         require(
             msg.sender == keeper ||
                 msg.sender == strategist ||
                 msg.sender == governance() ||
                 msg.sender == vault.guardian() ||
-                msg.sender == vault.management(),
-            "!authorized"
+                msg.sender == vault.management()
         );
-        _;
     }
 
     modifier onlyVaultManagers() {
@@ -381,7 +372,8 @@ abstract contract BaseStrategy {
      *  This may only be called by governance or the existing strategist.
      * @param _strategist The new address to assign as `strategist`.
      */
-    function setStrategist(address _strategist) external onlyAuthorized {
+    function setStrategist(address _strategist) external {
+        _onlyAuthorized();
         require(_strategist != address(0));
         strategist = _strategist;
         emit UpdatedStrategist(_strategist);
@@ -400,7 +392,8 @@ abstract contract BaseStrategy {
      *  This may only be called by governance or the strategist.
      * @param _keeper The new address to assign as `keeper`.
      */
-    function setKeeper(address _keeper) external onlyAuthorized {
+    function setKeeper(address _keeper) external {
+        _onlyAuthorized();
         require(_keeper != address(0));
         keeper = _keeper;
         emit UpdatedKeeper(_keeper);
@@ -414,7 +407,8 @@ abstract contract BaseStrategy {
      *  This may only be called by the strategist.
      * @param _rewards The address to use for pulling rewards.
      */
-    function setRewards(address _rewards) external onlyStrategist {
+    function setRewards(address _rewards) external {
+        _onlyStrategist();
         require(_rewards != address(0));
         vault.approve(rewards, 0);
         rewards = _rewards;
@@ -434,7 +428,8 @@ abstract contract BaseStrategy {
      *  This may only be called by governance or the strategist.
      * @param _delay The minimum number of seconds to wait between harvests.
      */
-    function setMinReportDelay(uint256 _delay) external onlyAuthorized {
+    function setMinReportDelay(uint256 _delay) external {
+        _onlyAuthorized();
         minReportDelay = _delay;
         emit UpdatedMinReportDelay(_delay);
     }
@@ -451,7 +446,8 @@ abstract contract BaseStrategy {
      *  This may only be called by governance or the strategist.
      * @param _delay The maximum number of seconds to wait between harvests.
      */
-    function setMaxReportDelay(uint256 _delay) external onlyAuthorized {
+    function setMaxReportDelay(uint256 _delay) external {
+        _onlyAuthorized();
         maxReportDelay = _delay;
         emit UpdatedMaxReportDelay(_delay);
     }
@@ -466,7 +462,8 @@ abstract contract BaseStrategy {
      * @param _profitFactor A ratio to multiply anticipated
      * `harvest()` gas cost against.
      */
-    function setProfitFactor(uint256 _profitFactor) external onlyAuthorized {
+    function setProfitFactor(uint256 _profitFactor) external {
+        _onlyAuthorized();
         profitFactor = _profitFactor;
         emit UpdatedProfitFactor(_profitFactor);
     }
@@ -484,7 +481,8 @@ abstract contract BaseStrategy {
      * @param _debtThreshold How big of a loss this Strategy may carry without
      * being required to report to the Vault.
      */
-    function setDebtThreshold(uint256 _debtThreshold) external onlyAuthorized {
+    function setDebtThreshold(uint256 _debtThreshold) external {
+        _onlyAuthorized();
         debtThreshold = _debtThreshold;
         emit UpdatedDebtThreshold(_debtThreshold);
     }
@@ -497,7 +495,8 @@ abstract contract BaseStrategy {
      *  This may only be called by governance or the strategist.
      * @param _metadataURI The URI that describe the strategy.
      */
-    function setMetadataURI(string calldata _metadataURI) external onlyAuthorized {
+    function setMetadataURI(string calldata _metadataURI) external {
+        _onlyAuthorized();
         metadataURI = _metadataURI;
         emit UpdatedMetadataURI(_metadataURI);
     }
@@ -665,7 +664,8 @@ abstract contract BaseStrategy {
      *
      *  This may only be called by governance, the strategist, or the keeper.
      */
-    function tend() external onlyKeepers {
+    function tend() external {
+        _onlyKeepers();
         // Don't take profits with this call, but adjust for better gains
         adjustPosition(vault.debtOutstanding());
     }
@@ -752,7 +752,8 @@ abstract contract BaseStrategy {
      *  called to report to the Vault on the Strategy's position, especially if
      *  any losses have occurred.
      */
-    function harvest() external onlyKeepers {
+    function harvest() external {
+        _onlyKeepers();
         uint256 profit = 0;
         uint256 loss = 0;
         uint256 debtOutstanding = vault.debtOutstanding();
@@ -844,7 +845,8 @@ abstract contract BaseStrategy {
      * @dev
      *  See `vault.setEmergencyShutdown()` and `harvest()` for further details.
      */
-    function setEmergencyExit() external onlyEmergencyAuthorized {
+    function setEmergencyExit() external {
+        _onlyEmergencyAuthorized();
         emergencyExit = true;
         vault.revokeStrategy();
 
@@ -888,7 +890,8 @@ abstract contract BaseStrategy {
      *  should be protected from sweeping in addition to `want`.
      * @param _token The token to transfer out of this vault.
      */
-    function sweep(address _token) external onlyGovernance {
+    function sweep(address _token) external {
+        _onlyGovernance();
         require(_token != address(want), "!want");
         require(_token != address(vault), "!shares");
 

tests/functional/strategy/test_config.py

@@ -74,14 +74,14 @@ def test_strategy_harvest_permission(
 
 
 @pytest.mark.parametrize(
-    "getter,setter,val,gov_allowed,strategist_allowed,authority_error",
+    "getter,setter,val,gov_allowed,strategist_allowed",
     [
-        ("rewards", "setRewards", None, False, True, "!strategist"),
-        ("keeper", "setKeeper", None, True, True, "!authorized"),
-        ("minReportDelay", "setMinReportDelay", 1000, True, True, "!authorized"),
-        ("maxReportDelay", "setMaxReportDelay", 2000, True, True, "!authorized"),
-        ("profitFactor", "setProfitFactor", 1000, True, True, "!authorized"),
-        ("debtThreshold", "setDebtThreshold", 1000, True, True, "!authorized"),
+        ("rewards", "setRewards", None, False, True),
+        ("keeper", "setKeeper", None, True, True),
+        ("minReportDelay", "setMinReportDelay", 1000, True, True),
+        ("maxReportDelay", "setMaxReportDelay", 2000, True, True),
+        ("profitFactor", "setProfitFactor", 1000, True, True),
+        ("debtThreshold", "setDebtThreshold", 1000, True, True),
     ],
 )
 def test_strategy_setParams(
@@ -94,7 +94,6 @@ def test_strategy_setParams(
     val,
     gov_allowed,
     strategist_allowed,
-    authority_error,
 ):
     if val is None:
         # Can't access fixtures, so use None to mean any random address
@@ -103,7 +102,7 @@ def test_strategy_setParams(
     prev_val = getattr(strategy, getter)()
 
     # None of these params can be set by a rando
-    with brownie.reverts(authority_error):
+    with brownie.reverts():
         getattr(strategy, setter)(val, {"from": rando})
 
     def try_setParam(caller, allowed):
@@ -114,7 +113,7 @@ def try_setParam(caller, allowed):
             getattr(strategy, setter)(prev_val, {"from": caller})
             assert getattr(strategy, getter)() == prev_val
         else:
-            with brownie.reverts(authority_error):
+            with brownie.reverts():
                 getattr(strategy, setter)(val, {"from": caller})
 
     try_setParam(strategist, strategist_allowed)
@@ -126,14 +125,14 @@ def test_set_strategist_authority(strategy, strategist, rando):
     # so this test handles it.
 
     # Only gov or strategist can setStrategist
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.setStrategist(rando, {"from": rando})
 
     # As strategist, set strategist to rando.
     strategy.setStrategist(rando, {"from": strategist})
 
     # Now the original strategist shouldn't be able to set strategist again
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.setStrategist(rando, {"from": strategist})
 
 

tests/functional/strategy/test_misc.py

@@ -9,7 +9,7 @@ def test_harvest_tend_authority(gov, keeper, strategist, strategy, rando, chain)
     strategy.tend({"from": keeper})
     strategy.tend({"from": strategist})
     strategy.tend({"from": gov})
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.tend({"from": rando})
 
     # Only keeper, strategist, or gov can call harvest
@@ -20,7 +20,7 @@ def test_harvest_tend_authority(gov, keeper, strategist, strategy, rando, chain)
     strategy.harvest({"from": strategist})
     chain.sleep(1)
     strategy.harvest({"from": gov})
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.harvest({"from": rando})
 
 
@@ -146,7 +146,7 @@ def test_sweep(gov, vault, strategy, rando, token, other_token):
     assert other_token.balanceOf(strategy) > 0
     assert other_token.balanceOf(gov) == 0
     # Not any random person can do this
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.sweep(other_token, {"from": rando})
 
     before = other_token.balanceOf(strategy)
@@ -184,7 +184,7 @@ def test_set_metadataURI(gov, strategy, strategist, rando):
     assert strategy.metadataURI() == "ipfs://test2"
     strategy.setMetadataURI("ipfs://test3", {"from": strategist})
     assert strategy.metadataURI() == "ipfs://test3"
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.setMetadataURI("ipfs://fake", {"from": rando})
 
 

tests/functional/strategy/test_shutdown.py

@@ -136,9 +136,9 @@ def test_set_emergency_exit_authority(
     strategy, gov, strategist, keeper, rando, management, guardian
 ):
     # Can only setEmergencyExit as governance, strategist, vault management and guardian
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.setEmergencyExit({"from": keeper})
-    with brownie.reverts("!authorized"):
+    with brownie.reverts():
         strategy.setEmergencyExit({"from": rando})
     strategy.setEmergencyExit({"from": gov})
     brownie.chain.undo()