Use swapFeeManager as owner in mev hook (#1305)

pkg/pool-hooks/contracts/MevCaptureHook.sol

@@ -244,7 +244,7 @@ contract MevCaptureHook is BaseHooks, SingletonAuthentication, VaultGuard, IMevC
     function setPoolMevTaxMultiplier(
         address pool,
         uint256 newPoolMevTaxMultiplier
-    ) external withMevTaxEnabledPool(pool) authenticate {
+    ) external withMevTaxEnabledPool(pool) onlySwapFeeManagerOrGovernance(pool) {
         _setPoolMevTaxMultiplier(pool, newPoolMevTaxMultiplier);
     }
 
@@ -279,7 +279,7 @@ contract MevCaptureHook is BaseHooks, SingletonAuthentication, VaultGuard, IMevC
     function setPoolMevTaxThreshold(
         address pool,
         uint256 newPoolMevTaxThreshold
-    ) external withMevTaxEnabledPool(pool) authenticate {
+    ) external withMevTaxEnabledPool(pool) onlySwapFeeManagerOrGovernance(pool) {
         _setPoolMevTaxThreshold(pool, newPoolMevTaxThreshold);
     }
 

pkg/pool-hooks/contracts/StableSurgeHook.sol

@@ -88,11 +88,6 @@ contract StableSurgeHook is BaseHooks, VaultGuard, SingletonAuthentication {
         _;
     }
 
-    modifier withPermission(address pool) {
-        _ensureValidSender(pool);
-        _;
-    }
-
     // Store the current threshold for each pool.
     mapping(address pool => uint256 threshold) private _surgeThresholdPercentage;
 
@@ -285,7 +280,7 @@ contract StableSurgeHook is BaseHooks, VaultGuard, SingletonAuthentication {
     function setMaxSurgeFeePercentage(
         address pool,
         uint256 newMaxSurgeSurgeFeePercentage
-    ) external withValidPercentage(newMaxSurgeSurgeFeePercentage) withPermission(pool) {
+    ) external withValidPercentage(newMaxSurgeSurgeFeePercentage) onlySwapFeeManagerOrGovernance(pool) {
         _setMaxSurgeFeePercentage(pool, newMaxSurgeSurgeFeePercentage);
     }
 
@@ -297,23 +292,10 @@ contract StableSurgeHook is BaseHooks, VaultGuard, SingletonAuthentication {
     function setSurgeThresholdPercentage(
         address pool,
         uint256 newSurgeThresholdPercentage
-    ) external withValidPercentage(newSurgeThresholdPercentage) withPermission(pool) {
+    ) external withValidPercentage(newSurgeThresholdPercentage) onlySwapFeeManagerOrGovernance(pool) {
         _setSurgeThresholdPercentage(pool, newSurgeThresholdPercentage);
     }
 
-    /// @dev Ensure the sender is the swapFeeManager, or default to governance if there is no manager.
-    function _ensureValidSender(address pool) private view {
-        address swapFeeManager = _vault.getPoolRoleAccounts(pool).swapFeeManager;
-
-        if (swapFeeManager == address(0)) {
-            if (_canPerform(getActionId(msg.sig), msg.sender, pool) == false) {
-                revert SenderNotAllowed();
-            }
-        } else if (swapFeeManager != msg.sender) {
-            revert SenderNotAllowed();
-        }
-    }
-
     /**
      * @notice Calculate the surge fee percentage. If below threshold, return the standard static swap fee percentage.
      * @dev It is public to allow it to be called off-chain.

pkg/pool-hooks/test/foundry/MevCaptureHook.t.sol

@@ -5,7 +5,12 @@ pragma solidity ^0.8.24;
 import "forge-std/Test.sol";
 
 import { IAuthentication } from "@balancer-labs/v3-interfaces/contracts/solidity-utils/helpers/IAuthentication.sol";
-import { PoolSwapParams, MAX_FEE_PERCENTAGE } from "@balancer-labs/v3-interfaces/contracts/vault/VaultTypes.sol";
+import {
+    PoolSwapParams,
+    MAX_FEE_PERCENTAGE,
+    PoolRoleAccounts
+} from "@balancer-labs/v3-interfaces/contracts/vault/VaultTypes.sol";
+import { IVaultExtension } from "@balancer-labs/v3-interfaces/contracts/vault/IVaultExtension.sol";
 import { IMevCaptureHook } from "@balancer-labs/v3-interfaces/contracts/pool-hooks/IMevCaptureHook.sol";
 import { IHooks } from "@balancer-labs/v3-interfaces/contracts/vault/IHooks.sol";
 import { IVault } from "@balancer-labs/v3-interfaces/contracts/vault/IVault.sol";
@@ -417,6 +422,19 @@ contract MevCaptureHookTest is BaseVaultTest {
         );
     }
 
+    function testSetPoolMevTaxMultiplierRevertIfSenderIsNotFeeManager() public {
+        _mockPoolRoleAccounts(address(0x01));
+
+        vm.expectRevert(IAuthentication.SenderNotAllowed.selector);
+        _mevCaptureHook.setPoolMevTaxMultiplier(pool, 5e18);
+    }
+
+    function testSetPoolMevTaxMultiplierWithSwapFeeManager() public {
+        _mockPoolRoleAccounts(address(this));
+
+        _mevCaptureHook.setPoolMevTaxMultiplier(pool, 5e18);
+    }
+
     /********************************************************
                    getPoolMevTaxThreshold()
     ********************************************************/
@@ -450,6 +468,19 @@ contract MevCaptureHookTest is BaseVaultTest {
         _mevCaptureHook.setPoolMevTaxThreshold(pool, 5e18);
     }
 
+    function testSetPoolMevTaxThresholdRevertIfSenderIsNotFeeManager() public {
+        _mockPoolRoleAccounts(address(0x01));
+
+        vm.expectRevert(IAuthentication.SenderNotAllowed.selector);
+        _mevCaptureHook.setPoolMevTaxThreshold(pool, 5e18);
+    }
+
+    function testSetPoolMevTaxThresholdWithSwapFeeManager() public {
+        _mockPoolRoleAccounts(address(this));
+
+        _mevCaptureHook.setPoolMevTaxThreshold(pool, 5e18);
+    }
+
     function testSetPoolMevTaxThreshold() public {
         uint256 firstPoolMevTaxThreshold = _mevCaptureHook.getPoolMevTaxThreshold(pool);
         uint256 firstDefaultMevTaxThreshold = _mevCaptureHook.getDefaultMevTaxThreshold();
@@ -731,4 +762,20 @@ contract MevCaptureHookTest is BaseVaultTest {
         assertFalse(_mevCaptureHook.isMevTaxExemptSender(bob), "Bob is exempt");
         assertTrue(_mevCaptureHook.isMevTaxExemptSender(alice), "Alice is not exempt");
     }
+
+    /********************************************************
+                            Other
+    ********************************************************/
+    function _mockPoolRoleAccounts(address swapFeeManager) private {
+        PoolRoleAccounts memory poolRoleAccounts = PoolRoleAccounts({
+            pauseManager: address(0x01),
+            swapFeeManager: swapFeeManager,
+            poolCreator: address(0x01)
+        });
+        vm.mockCall(
+            address(vault),
+            abi.encodeWithSelector(IVaultExtension.getPoolRoleAccounts.selector, pool),
+            abi.encode(poolRoleAccounts)
+        );
+    }
 }

pkg/vault/contracts/SingletonAuthentication.sol

@@ -16,6 +16,12 @@ import { Authentication } from "@balancer-labs/v3-solidity-utils/contracts/helpe
 abstract contract SingletonAuthentication is Authentication {
     IVault private immutable _vault;
 
+    modifier onlySwapFeeManagerOrGovernance(address pool) {
+        address roleAddress = _vault.getPoolRoleAccounts(pool).swapFeeManager;
+        _ensureAuthenticatedByExclusiveRole(pool, roleAddress);
+        _;
+    }
+
     // Use the contract's own address to disambiguate action identifiers.
     constructor(IVault vault) Authentication(bytes32(uint256(uint160(address(this))))) {
         _vault = vault;
@@ -44,4 +50,16 @@ abstract contract SingletonAuthentication is Authentication {
     function _canPerform(bytes32 actionId, address account, address where) internal view returns (bool) {
         return getAuthorizer().canPerform(actionId, account, where);
     }
+
+    /// @dev Ensure the sender is the roleAddress, or default to governance if roleAddress is address(0).
+    function _ensureAuthenticatedByExclusiveRole(address pool, address roleAddress) internal view {
+        if (roleAddress == address(0)) {
+            // Defer to governance if no role assigned.
+            if (_canPerform(getActionId(msg.sig), msg.sender, pool) == false) {
+                revert SenderNotAllowed();
+            }
+        } else if (msg.sender != roleAddress) {
+            revert SenderNotAllowed();
+        }
+    }
 }