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

Missing validation of `prev` in `_uninstallValidator` can corrupt the validator linked list

benrai

Lines of code

https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/base/ModuleManager.sol#L216-L227

Impact

The missing validation in _uninstallValidator can lead to the corruption of the linked list. Specifically, if the prev address provided does not correctly point to the validator to be removed, the linked list structure will be broken. This can result in:

  • Inability to correctly traverse the list.

  • Incorrect data being returned by functions that rely on the list, such as getValidatorsPaginated.

Proof of Concept

The ModuleManager contract manages various modules, including validators, using a linked list structure provided by the SentinelListLib library. The _uninstallValidator function is responsible for removing a validator from this list. However, since the prev address in the provided data can be freely set, a validation should be done to ensure the integrity of the linked list after the removal of the validator. If a wrong prev address is provided, because of the missing check, the linked list will be broken.

Example:

  1. Assume the linked list of validators is as follows: SENTINEL -> Validator1 -> Validator2 -> Validator3 -> SENTINEL.

  2. A call to _uninstallValidator is made with prev = Validator1 and validator = Validator3.

  3. The function does not check if validators.getNext(Validator1) equals Validator3.

  4. The pop function is called, which sets self.entries[Validator1] to self.entries[Validator3], effectively incorrectly removing Validator2 from the list and corrupting the structure.

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

Relevant code:

function _uninstallValidator(address validator, bytes calldata data) internal virtual {
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
​
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
// Check if the account has at least one validator installed before proceeding
// Having at least one validator is a requirement for the account to function properly
require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator());
validators.pop(prev, validator);
IValidator(validator).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 validation step to ensure that validators.getNext(prev) equals validator before proceeding with the removal. This ensures that the prev address correctly points to the validator to be removed, maintaining the integrity of the linked list.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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