RSETH is guarded by two possibly disagreeing access control systems

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/RSETH.sol#L19 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/RSETH.sol#L16 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/RSETH.sol#L73

Vulnerability details

Impact

A critical issue in the RSETH contract arises from a conflict between two access control systems operating simultaneously, leading to confusing and possibly inconsistent privileges. The RSETH contract inherits from AccessControlUpgradeable and additionally holds a reference to an LRTConfig, which itself inherits from AccessControlUpgradeable. This dual-system approach (inheritance and composition) results in discrepancies in permissions across various functions.

The inherited access control in RSETH guards key functions like mint, burnFrom, unpause, and updateLRTConfig. Conversely, the pause function relies on the access control system of the LRTConfig. The core problem lies in these two systems not aligning on who holds the administrator role.

In other words, the two systems can assign roles to different addresses. For example, the inherited system grants Alice the DEFAULT_ADMIN_ROLE, while the composed system (from LRTConfig) assigns the DEFAULT_ADMIN_ROLE only to Bob. Depending on which system you asked, you will get different answers to "Is Alice an admin?".

Proof of Concept

Consider the following scenario:

1. Kelp attempts to transfer admin privileges to a new account for a different multi-sig setup.
2. Kelp assigns the DEFAULT_ADMIN_ROLE to the new address in LRTConfig.
3. Kelp revokes the DEFAULT_ADMIN_ROLE from the old address in LRTConfig.

There are two problematic consequences of this action:

1. The old admin retains the permission to operate the RSETH contract, including replacing the configuration.
2. The new admin cannot perform crucial operations on the RSETH contract, such as unpausing or altering the configuration.

Here are two test cases to trigger these scenarios respectively.

diff --git a/test/RSETHTest.t.sol b/test/RSETHTest.t.sol
index a0047a8..96e31f8 100644
--- a/test/RSETHTest.t.sol
+++ b/test/RSETHTest.t.sol
@@ -260,3 +260,49 @@ contract RSETHUpdateLRTConfig is RSETHTest {
         assertEq(address(newLRTConfig), address(rseth.lrtConfig()), "LRT config address is not set");
     }
 }
+
+contract RSETHAccessControlBug is RSETHTest {
+
+    function test_access_control_violation1() external {
+        bytes32 DEFAULT_ADMIN_ROLE = 0x0;
+        address newAdmin = makeAddr("newAdmin");
+        rseth.initialize(address(admin), address(lrtConfig));
+
+        // Step 1: Grant privilges to new admin
+        vm.prank(admin);
+        lrtConfig.grantRole(DEFAULT_ADMIN_ROLE, newAdmin);
+
+        // Step 2: Revoke privilges from old admin
+        vm.prank(newAdmin);
+        lrtConfig.grantRole(DEFAULT_ADMIN_ROLE, admin);
+
+        // Step 3: Use old admin to replace the configuration of RSETH
+        vm.prank(admin);
+        vm.expectRevert();
+        rseth.updateLRTConfig(address(0x1));
+    }
+
+    function test_access_control_violation2() external {
+        bytes32 DEFAULT_ADMIN_ROLE = 0x0;
+        address newAdmin = makeAddr("newAdmin");
+        rseth.initialize(address(admin), address(lrtConfig));
+        vm.prank(admin);
+        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
+
+        // Step 1: Grant privilges to new admin
+        vm.prank(admin);
+        lrtConfig.grantRole(DEFAULT_ADMIN_ROLE, newAdmin);
+
+        // Step 2: Revoke privilges from old admin
+        vm.prank(newAdmin);
+        lrtConfig.grantRole(DEFAULT_ADMIN_ROLE, admin);
+
+        // Step 3: Manager pauses the contract
+        vm.prank(manager);
+        rseth.pause();
+
+        // Step 4: New admin tries to unpause the contract
+        vm.prank(newAdmin);
+        rseth.unpause();
+    }
+}

Use forge test --match-test test_access_control_violation to reproduce. The first test fails because it's not reverting as expected in the last line. The second test fails because it is unexpectedly reverting in the last line.

forge test --match-test test_access_control_violation                                     [17:57:43]
[⠒] Compiling...
No files changed, compilation skipped

Running 2 tests for test/RSETHTest.t.sol:RSETHAccessControlBug
[FAIL. Reason: call did not revert as expected] test_access_control_violation1() (gas: 165722)
[FAIL. Reason: revert: AccessControl: account 0x67ed0ac45b537a79406e01656d90659325455585 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000] test_access_control_violation2() (gas: 248761)
Test result: FAILED. 0 passed; 2 failed; 0 skipped; finished in 2.32ms

Ran 1 test suites: 0 tests passed, 2 failed, 0 skipped (2 total tests)

Failing tests:
Encountered 2 failing tests in test/RSETHTest.t.sol:RSETHAccessControlBug
[FAIL. Reason: call did not revert as expected] test_access_control_violation1() (gas: 165722)
[FAIL. Reason: revert: AccessControl: account 0x67ed0ac45b537a79406e01656d90659325455585 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000] test_access_control_violation2() (gas: 248761)

Encountered a total of 2 failing tests, 0 tests succeeded
FAIL

Recommended Mitigation Steps

Eliminate the dual access control system in the RSETH contract. Ensure a consistent access control mechanism is used for all administrative functions to avoid conflicting permissions.

Tools used

Foundry

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

manoj9april marked the issue as disagree with severity

[gus-stdr]

Thank you for submitting the review. We believe this issue is not of high severity given that we own the addresses that manages the access control of RSETH. It falls into a QA category of severity.

[c4-sponsor]

manoj9april (sponsor) confirmed

[fatherGoose1]

Agree that this constitutes QA.

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b