Jolly Malachite Anteater - The _removeSigner function lacks validation to ensure that the signer being removed has no active responsibilities or critical roles in the system.

[sherlock-admin2]

Jolly Malachite Anteater

Medium

The _removeSigner function lacks validation to ensure that the signer being removed has no active responsibilities or critical roles in the system.

Summary

The missing validation in the _removeSigner function can disrupt the system's functionality, as it may remove a signer who holds active responsibilities or critical roles within the Safe. This oversight occurs because the function directly removes a signer and deletes their associated data from registeredSignerHats, without ensuring that they are no longer required for ongoing tasks or vital system operations.

Root Cause

HatsSignerGate.sol#l756

In the _removeSigner function (line X in the contract), the implementation removes a signer and deletes any associated data from registeredSignerHats, without checking whether:

1.The signer is still actively performing critical duties.
2.The signer has roles or responsibilities that are essential for the proper functioning of the Safe.

For instance:
A critical signer responsible for approving important transactions could be removed, causing a disruption.
Associated roles or data linked to the signer are deleted without verifying their current necessity.
This lack of validation can leave the system in an inconsistent or broken state.

Internal pre-conditions

1.A signer (_signer) is registered in the Safe and has data stored in registeredSignerHats.
2.The signer is performing active tasks (e.g., transaction approval or serving as a required participant in a multi-signature process).
3.The _removeSigner function is called to remove the signer, without validating their responsibilities.

Impact

If this issue is not addressed, the following consequences may occur:

Disruption of System Functionality: If the removed signer is critical to ongoing operations (e.g., approving transactions), the system may fail to execute necessary actions.
Loss of System Integrity: Removing a signer who is actively participating in the Safe or has important roles may leave the system in an invalid or inconsistent state.
Data Inconsistencies: Deleting the signer's associated data (registeredSignerHats) without validation can lead to a loss of important role-related information.

PoC

A scenario illustrating how the lack of validation in _removeSigner can disrupt the system:

1.A Safe is initialized with a critical signer who is required for approving multi-signature transactions.
2.The _removeSigner function is called to remove this signer, without validating whether they have active roles or responsibilities.
3.The critical signer is removed, causing disruptions such as:
Incomplete transaction approvals.
Unmet quorum requirements for multi-signature operations.
Loss of associated role information in registeredSignerHats.

contract Example {
    function testRemoval() public {
        address criticalSigner = 0x123...; // Assume this signer is essential for transaction approvals.
        _removeSigner(criticalSigner);    // This removes the critical signer without validation.
        // Subsequent transaction approvals may fail due to the missing signer.
    }
}

Proposed Solution

To mitigate this issue, the _removeSigner function must include proper validation to ensure that the signer being removed has no active responsibilities or critical roles.

Step 1: Add Validation Before Removal Include a require statement to validate that the signer has no active responsibilities:

require(_signerHasNoActiveResponsibilities(_signer), "Signer has active responsibilities and cannot be removed");

Step 2: Implement a Helper Function Define a helper function to check the signer's status:

function _signerHasNoActiveResponsibilities(address _signer) internal view returns (bool) {
    return registeredSignerHats[_signer] == 0 && !_isCriticalSigner(_signer);
}

Step 3: Define Critical Role Check Implement the _isCriticalSigner function to verify whether the signer has any critical roles in the system:

function _isCriticalSigner(address _signer) internal view returns (bool) {
    // Example: Check if the signer is part of a quorum or required for specific actions
    return safe.isOwner(_signer) && safe.getOwners().length <= safe.getThreshold();
}

Mitigation

1.Validation Layer: Ensure that the _removeSigner function validates the signer's responsibilities before performing the removal.
2.Role Management: Implement a clear system to track whether a signer is critical or has pending tasks before allowing their removal.
3.Failsafe Mechanism: If the signer cannot be removed safely, provide feedback to the system (e.g., via revert messages) to indicate why the removal is blocked.