076.md

Allarious

high

[High][Flow] reconcileSignerCount is not updating the safe threshold correctly

Summary

reconcileSignerCount which is one of the most important functions in HSG is not implementing the call to changeThreshold correctly.

Vulnerability Detail

reconcileSignerCount is called whenever HSG needs to be synced with the valid signers that are active in the safe contract and are wearing a hat. However, the function call to safe to set the correct threshold, is implemented incorrectly:

bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L204

While newThreshold holds the correct value for the safe threshold, validSignerCount is passed to the safe which can be more than targetThreshold and is not the correct amount. This is probably happened from the changes implemented from the last private audit provided in the description of the project. (TRST-H-5 Minority may be able to call safe operations)

Imagine the scenario below: (1) There are currently 5 signers, target threshold is set to 7, and the current threshold is 5 (2) The number of signers increase to 9 (3) The threshold of the safe should be set to 7 as it is the target threshold, but it gets set to 9, which is the number of valid signers

Impact

This can cause the safe threshold to ignore the targetThreshold and cause valid number of signatures to be rejected by the system. reconcileSignerCount is supposed to be one of the core functions that keeps things updated between safe and HSG, with such a bug, many security assumptions will be broken.

Code Snippet

Code snippet not provided.

Tool used

Manual Review

Recommendation

The diff below should be implemented:

@line 204 HatSignerGateBase.sol
- bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
+ bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);