Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Missing validation of `prev` entry in `_uninstallExecutor` can corrupt linked list

benrai

Lines of code

Impact

The missing validation of the prev entry in the _uninstallExecutor function can lead to corruption of the linked list of executors. This can have several consequences:

  1. The linked list structure can be broken, leading to incorrect traversal and data retrieval.

  2. Functions relying on the integrity of the linked list, such as getExecutorsPaginated, will return incorrect data.

Proof of Concept

The _uninstallExecutor function in ModuleManager.sol is responsible for removing an executor from the linked list. It decodes the data parameter to get the prev entry and the disableModuleData. It then calls the pop function of the SentinelList library to remove the executor from the executors linked list.

However, there is no check to ensure that the prev entry actually points to the executor to be removed. This can lead to a situation where an incorrect prev entry is provided, causing the linked list to be corrupted.

Example Scenario

  1. Assume the linked list of validators is as follows: SENTINEL -> Executor1 -> Executor2 -> Executor3 -> SENTINEL.

  2. A call to _uninstallValidator is made with prev = Executor1 and validator = Executor3.

  3. The function does not check if validators.getNext(Executor1) equals Executor3.

  4. The pop function is called, which sets self.entries[Executor1] to self.entries[Executor3]which is SENTINEL

  5. The resulting list will look like: SENTINEL -> Validator1 -> SENTINEL, with Validator2 incorrectly removed.

Relevant Code

function uninstallExecutor(address executor, bytes calldata data) internal virtual {
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
getAccountStorage().executors.pop(prev, executor);
IExecutor(executor).onUninstall(disableModuleData);
}
function pop(SentinelList storage self, address prevEntry, address popEntry) internal {
if (popEntry == ZERO_ADDRESS || popEntry == SENTINEL) {
revert LinkedList_InvalidEntry(prevEntry);
}
if (self.entries[prevEntry] != popEntry) revert LinkedList_InvalidEntry(popEntry);
self.entries[prevEntry] = self.entries[popEntry];
self.entries[popEntry] = ZERO_ADDRESS;
}
​

Recommended Mitigation Steps

Add a check in the _uninstallExecutor function to ensure that the prev entry points to the executor before calling the pop function. This can be done by verifying that executors.getNext(prev) == executor.

Suggested Code Fix

function uninstallExecutor(address executor, bytes calldata data) internal virtual {
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
require(getAccountStorage().executors.getNext(prev) == executor, "Invalid prev entry");
getAccountStorage().executors.pop(prev, executor);
IExecutor(executor).onUninstall(disableModuleData);
}
Updates

Lead Judging Commences

0xnevi Lead Judge
12 months ago
0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.