access control fix

etherfi-protocol · Dec 20, 2024 · 17778b9 · 17778b9
1 parent 70b62bd
commit 17778b9
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 14 deletions.
diff --git a/script/deploys/DeployEtherFiOperationParameters.s.sol b/script/deploys/DeployEtherFiOperationParameters.s.sol
@@ -23,16 +23,24 @@ contract Deploy is Script {
 
         vm.startBroadcast(deployerPrivateKey);
 
-        EtherFiOperationParameters impl = new EtherFiOperationParameters();
-        UUPSProxy proxy = new UUPSProxy(address(impl), "");
-
-        EtherFiOperationParameters instance = EtherFiOperationParameters(payable(proxy));
-
-        instance.updateTagAdmin("ORACLE", 0x566E58ac0F2c4BCaF6De63760C56cC3f825C48f5, true);
-        instance.updateTagAdmin("EIGENPOD", 0x566E58ac0F2c4BCaF6De63760C56cC3f825C48f5, true);
-        // instance.updateTagKeyValue("ORACLE", "WITHDRAWAL_TARGET_GAS_PRICE", "12.5"); // 12.5 gwei
-        // instance.updateTagKeyValue("ORACLE", "TARGET_LIQUIDITY_IN_PERCENT_OF_TVL", "2.0"); // 2.0 % of TVL
-        // instance.updateTagKeyValue("EIGENPOD", "PROOF_SUBMIT_TARGET_GAS_PRICE", "12.5"); // 12.5 gwei
+        bool to_upgrade = true;
+
+        if (to_upgrade) {
+            EtherFiOperationParameters instance = EtherFiOperationParameters(0xD0Ff8996DB4bDB46870b7E833b7532f484fEad1A);
+            EtherFiOperationParameters impl = new EtherFiOperationParameters();
+            instance.upgradeTo(address(impl));
+        } else {
+            EtherFiOperationParameters impl = new EtherFiOperationParameters();
+            UUPSProxy proxy = new UUPSProxy(address(impl), "");
+
+            EtherFiOperationParameters instance = EtherFiOperationParameters(payable(proxy));
+
+            instance.updateTagAdmin("ORACLE", 0x566E58ac0F2c4BCaF6De63760C56cC3f825C48f5, true);
+            instance.updateTagAdmin("EIGENPOD", 0x566E58ac0F2c4BCaF6De63760C56cC3f825C48f5, true);
+            // instance.updateTagKeyValue("ORACLE", "WITHDRAWAL_TARGET_GAS_PRICE", "12.5"); // 12.5 gwei
+            // instance.updateTagKeyValue("ORACLE", "TARGET_LIQUIDITY_IN_PERCENT_OF_TVL", "2.0"); // 2.0 % of TVL
+            // instance.updateTagKeyValue("EIGENPOD", "PROOF_SUBMIT_TARGET_GAS_PRICE", "12.5"); // 12.5 gwei
+        }
 
         vm.stopBroadcast();
     }

diff --git a/src/helpers/EtherFiOperationParameters.sol b/src/helpers/EtherFiOperationParameters.sol
@@ -16,18 +16,18 @@ contract EtherFiOperationParameters is UUPSUpgradeable, OwnableUpgradeable {
         _disableInitializers();
     }
 
-    function initialize() public initializer {
+    function initialize() external initializer {
         __Ownable_init();
         __UUPSUpgradeable_init();
     }
 
-    function updateTagAdmin(string memory tag, address admin, bool allowed) public {
+    function updateTagAdmin(string memory tag, address admin, bool allowed) external onlyOwner {
         tagAdmins[tag][admin] = allowed;
 
         emit UpdatedAdmin(tag, admin, allowed);
     }
 
-    function updateTagKeyValue(string memory tag, string memory key, string memory value) public {
+    function updateTagKeyValue(string memory tag, string memory key, string memory value) external onlyAdmin(tag) {
         string memory old_value = tagKeyValues[tag][key];
         tagKeyValues[tag][key] = value;
 
@@ -39,4 +39,9 @@ contract EtherFiOperationParameters is UUPSUpgradeable, OwnableUpgradeable {
     function getImplementation() external view returns (address) {
         return _getImplementation();
     }
+
+    modifier onlyAdmin(string memory tag) {
+        require(tagAdmins[tag][msg.sender], "Only admin can call");
+        _;
+    }
 }
diff --git a/test/EtherFiOperationParameters.t.sol b/test/EtherFiOperationParameters.t.sol
@@ -10,6 +10,8 @@ import "../src/UUPSProxy.sol";
 contract EtherFiOperationParametersTest is TestSetup {
 
     EtherFiOperationParameters operationParameters;
+    address rando1 = address(0x1); // Non-owner account for testing
+    address rando2 = address(0x2);   // Another non-owner account
 
     function setUp() public {
         EtherFiOperationParameters impl = new EtherFiOperationParameters();
@@ -29,26 +31,98 @@ contract EtherFiOperationParametersTest is TestSetup {
     }
 
     function testFuzz_updateTagKeyValue(
+        string calldata tag,
+        string calldata key,
+        string calldata value1,
+        string calldata value2
+    ) public {
+        vm.expectRevert();
+        operationParameters.updateTagKeyValue(tag, key, value1);
+
+        vm.prank(operationParameters.owner());
+        operationParameters.updateTagAdmin(tag, admin, true);
+
+        vm.prank(admin);
+        operationParameters.updateTagKeyValue(tag, key, value1);
+        assertEq(operationParameters.tagKeyValues(tag, key), value1);
+
+        vm.prank(admin);
+        operationParameters.updateTagKeyValue(tag, key, value2);
+        assertEq(operationParameters.tagKeyValues(tag, key), value2);
+    }
+
+    // Test that only the owner can call updateTagAdmin
+    function testFuzz_updateTagAdmin_onlyOwner(
+        string calldata tag,
+        address admin,
+        bool allowed
+    ) public {
+        vm.prank(rando1); // Attempt to call from a non-owner account
+        vm.expectRevert("Ownable: caller is not the owner");
+        operationParameters.updateTagAdmin(tag, admin, allowed);
+
+        // Owner can successfully call the function
+        vm.prank(operationParameters.owner());
+        operationParameters.updateTagAdmin(tag, admin, allowed);
+        assertEq(operationParameters.tagAdmins(tag, admin), allowed);
+    }
+
+    // Test that only an admin can call updateTagKeyValue
+    function testFuzz_updateTagKeyValue_onlyAdmin(
         string calldata tag,
         string calldata key,
         string calldata value
     ) public {
+        // Attempt to call without being an admin
+        vm.prank(rando1);
+        vm.expectRevert("Only admin can call");
+        operationParameters.updateTagKeyValue(tag, key, value);
+
+        // Assign admin role to rando1 for the tag
+        vm.prank(operationParameters.owner());
+        operationParameters.updateTagAdmin(tag, rando1, true);
+
+        // Alice can now update the key value
+        vm.prank(rando1);
         operationParameters.updateTagKeyValue(tag, key, value);
         assertEq(operationParameters.tagKeyValues(tag, key), value);
     }
 
-    function testFuzz_updateTagKeyValue(
+    // Test the updateTagKeyValue function with multiple updates by an admin
+    function testFuzz_updateTagKeyValue_asAdmin(
         string calldata tag,
         string calldata key,
         string calldata value1,
         string calldata value2
     ) public {
+        // Assign admin role to rando2 for the tag
+        operationParameters.updateTagAdmin(tag, rando2, true);
+
+        // Bob updates the key value
+        vm.prank(rando2);
         operationParameters.updateTagKeyValue(tag, key, value1);
         assertEq(operationParameters.tagKeyValues(tag, key), value1);
+
+        // Bob updates the key value again
+        vm.prank(rando2);
         operationParameters.updateTagKeyValue(tag, key, value2);
         assertEq(operationParameters.tagKeyValues(tag, key), value2);
     }
 
+    // Test that only the owner can upgrade the contract
+    function test_upgrade_onlyOwner() public {
+        address newImplementation = address(new EtherFiOperationParameters());
+
+        // Attempt to upgrade from a non-owner account
+        vm.prank(rando1);
+        vm.expectRevert("Ownable: caller is not the owner");
+        operationParameters.upgradeTo(newImplementation);
+
+        // Owner upgrades successfully
+        operationParameters.upgradeTo(newImplementation);
+        assertEq(operationParameters.getImplementation(), newImplementation);
+    }
+
     function test_upgrade() public {
         address newImplementation = address(new EtherFiOperationParameters());
         operationParameters.upgradeTo(newImplementation);