0xKann - Ownership and Threshold Management Issue when First Signer Joins

[sherlock-admin2]

0xKann

Medium

Ownership and Threshold Management Issue when First Signer Joins

Summary

The current implementation of the claimSignersFor function does not account for the change in the ownership count and threshold when the first signer is added to the contract. Specifically, when the first user joins as a signer, ownership is transferred to them from the contract, but the change in ownership count (newNumOwners++) is not correctly recorded in the system

Root Cause

The claimSignersFor function checks whether the contract is the only owner at the beginning, which helps identify the first signer. However, while the first signer is added, the function does not update the ownership count (newNumOwners) until later in the loop. This results in an inaccurate count of owners and the corresponding threshold adjustments.
The code attempts to update the threshold only after the first signer is added, but there is no immediate tracking of the new ownership count when the ownership is transferred from the contract to the first signer.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

https://github.com/sherlock-audit/2024-11-hats-protocol/blob/main/hats-zodiac/src/HatsSignerGate.sol#L721-L746
First Signer Joins:

When the first signer is added, the contract needs to update the owner count and the threshold accordingly.
However, the newNumOwners variable, which tracks the new number of owners, is only incremented after the first signer is processed in the loop.
This creates a situation where the threshold may not be updated properly for the first signer, as the change in ownership is not reflected in the newNumOwners count at the right time.
Threshold Inaccuracy:

The threshold is based on the current number of owners. If newNumOwners is not incremented immediately when the first signer takes ownership, the threshold could be calculated incorrectly.
This may lead to incorrect behavior in the contract, as subsequent signers could be added with an improper threshold, potentially allowing more signers than expected or changing the quorum for decisions.

Impact

Threshold Manipulation: If the ownership count is not updated correctly when the first signer joins, it could lead to an inaccurate threshold calculation. This can undermine the integrity of the quorum requirements for the contract, as future signers may be able to bypass threshold checks.

PoC

No response

Mitigation

Update the count

if (i == 0 && isInitialOwnersState) {
  addOwnerData = SafeManagerLib.encodeSwapOwnerAction(SafeManagerLib.SENTINELS, address(this), signer);
  newNumOwners++;  // Update ownership count immediately for the first signer