Joyous Cobalt Chameleon - Threshold Violation When Removing a Signer

[sherlock-admin2]

Joyous Cobalt Chameleon

High

Threshold Violation When Removing a Signer

Summary

The threshold invariant (threshold >= min) is not maintained when the number of owners equals the min value. Specifically, if the number of owners equals min and a signer is removed, the _getNewThreshold function recalculates the threshold based on numOwners - 1. This can result in the threshold dropping below min, violating the intended security guarantees.

Root Cause

The lack of enforcement for the threshold >= min invariant will cause a critical reduction in security for the Safe. When the number of owners equals the configured min and a signer is removed, the threshold recalculation logic allows the threshold to fall below min. This can enable unauthorized actions, as fewer signatures than intended would be required to execute transactions.

-Link to code :

• https://github.com/sherlock-audit/2024-11-hats-protocol/blob/main/hats-zodiac/src/HatsSignerGate.sol#L282C1-L287C4
• https://github.com/sherlock-audit/2024-11-hats-protocol/blob/main/hats-zodiac/src/HatsSignerGate.sol#L763
• https://github.com/sherlock-audit/2024-11-hats-protocol/blob/main/hats-zodiac/src/HatsSignerGate.sol#L801C1-L808C4

Internal pre-conditions

-Number of Owners:

• The initial number of owners (numOwners) equals min.

-second condition:

• remove a signer

External pre-conditions

No response

Attack Path

-The Safe is initialized with the following configuration for example:

• min = 3
• numOwners = 3 (e.g., [Owner A, Owner B, Owner C]).

-An attacker or authorized signer removes one signer by calling removeSigner:HatsSignerGate (e.g., Owner C).
-The _getNewThreshold function recalculates the threshold:

• _getRequiredValidSignatures(2) returns min = 3 because numOwners < min.
  -The logic in _getNewThreshold checks if (_threshold > numOwners), which is true (3 > 2).
• The threshold is updated to numOwners = 2, violating the min constraint.

-The Safe's multisig configuration is now insecure, allowing a reduced number of signatures to execute critical transactions.

Impact

The protocol suffers a security breakdown as the multisig threshold drops below the intended min. This allows malicious actors to exploit weakened quorum requirements, potentially leading to unauthorized execution of transactions.

PoC

-Steps

• Deploy the following contract in Remix using Solidity version ^0.8.0.
• Initialize the variables min and numOwners to 3.
• Call the removeSigner() function to simulate the removal of a signer and observe the resulting threshold violation.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract ThresholdBugPoC {
    uint256 public min = 3; // Minimum threshold
    uint256 public numOwners = 3; // Initial number of owners
    uint256 public threshold;

    constructor() {
        // Set initial threshold
        threshold = getNewThreshold(numOwners);
    }

    function getRequiredValidSignatures(uint256 _numOwners) public view returns (uint256) {
        if (_numOwners < min) return min;
        return _numOwners;
    }

    function getNewThreshold(uint256 _numOwners) public view returns (uint256) {
        uint256 _threshold = getRequiredValidSignatures(_numOwners);
        if (_threshold > _numOwners) {
            _threshold = _numOwners; // Threshold is reduced to numOwners - 1 when numOwners < min
        }
        return _threshold;
    }

    function removeSigner() public {
        require(numOwners > 0, "No signers left to remove");
        numOwners--; // Simulate removing a signer
        threshold = getNewThreshold(numOwners); // Recalculate the threshold
    }
}

Mitigation

-Modify _getNewThreshold to explicitly enforce the min constraint during threshold recalculations:

function _getNewThreshold(uint256 numOwners) internal view returns (uint256 _threshold) {
    // Calculate the new threshold
    _threshold = _getRequiredValidSignatures(numOwners);

    // Ensure the threshold does not exceed the number of owners
    if (_threshold > numOwners) {
        _threshold = numOwners;
    }

    // Ensure the threshold is not less than the minimum
   if (_threshold < config.min) {
     _threshold = config.min;
    }
}