diff --git a/contracts/BaseStrategy.sol b/contracts/BaseStrategy.sol index d935f97d..cf8167b4 100644 --- a/contracts/BaseStrategy.sol +++ b/contracts/BaseStrategy.sol @@ -272,39 +272,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() ); - _; } constructor(address _vault) public { @@ -354,7 +345,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); @@ -373,7 +365,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); @@ -387,7 +380,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; @@ -407,7 +401,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); } @@ -424,7 +419,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); } @@ -439,7 +435,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); } @@ -457,7 +454,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); } @@ -470,7 +468,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); } @@ -638,7 +637,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()); } @@ -725,7 +725,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(); @@ -809,7 +810,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(); @@ -853,7 +855,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"); diff --git a/tests/functional/strategy/test_config.py b/tests/functional/strategy/test_config.py index 3bce69b3..2d7d2b22 100644 --- a/tests/functional/strategy/test_config.py +++ b/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}) diff --git a/tests/functional/strategy/test_misc.py b/tests/functional/strategy/test_misc.py index 60a55745..094a1ab9 100644 --- a/tests/functional/strategy/test_misc.py +++ b/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}) diff --git a/tests/functional/strategy/test_shutdown.py b/tests/functional/strategy/test_shutdown.py index e0e5d4b8..a8bf7e14 100644 --- a/tests/functional/strategy/test_shutdown.py +++ b/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()