Open sherlock-admin opened 1 year ago
There is no admin action that can revoke the validity of their hats, so the reconcileSignerCount() function will always revert, and therefore the safe is unusable.
This is not fully true, since either the hat's eligibility or toggle module could revoke their hat. But this is definitely not guaranteed to be possible, especially in the case of mechanistic modules that have hardcoded revocation logic, so addressing this issue is warranted.
My current thinking is that storing/comparing the full owner array — which is likely necessary to address #118 and #70 — would also address this issue, since ensuring exact same owners also ensures same number of owners.
cc @zobront
zobront added "Fix Approved" label
obront
high
Signers can brick safe by adding unlimited additional signers while avoiding checks
Summary
There are a number of checks in
checkAfterExecution()
to ensure that the signers cannot perform any illegal actions to exert too much control over the safe. However, there is no check to ensure that additional owners are not added to the safe. This could be done in a way that pushes the total overmaxSigners
, which will cause all future transactions to revert.This means that signers can easily collude to freeze the contract, giving themselves the power to hold the protocol ransom to unfreeze the safe and all funds inside it.
Vulnerability Detail
When new owners are added to the contract through the
claimSigner()
function, the total number of owners is compared tomaxSigners
to ensure it doesn't exceed it.However, owners can also be added by a normal
execTransaction
function. In this case, there are very few checks (all of which could easily or accidentally be missed) to stop us from adding too many owners:That means that either in the case that (a) the safe's threshold is already at
targetThreshold
or (b) the owners being added are currently toggled off or have eligibility turned off, this check will pass and the owners will be added.Once they are added, all future transactions will fail. Each time a transaction is processed,
checkTransaction()
is called, which callsreconcileSignerCount()
, which has the following check:This will revert as long as the new owners are now activated as valid signers.
In the worst case scenario, valid signers wearing an immutable hat are added as owners when the safe's threshold is already above
targetThreshold
. The check passes, but the new owners are already valid signers. There is no admin action that can revoke the validity of their hats, so thereconcileSignerCount()
function will always revert, and therefore the safe is unusable.Since
maxSigners
is immutable and can't be changed, the only solution is for the hat wearers to renounce their hats. Otherwise, the safe will remain unusable with all funds trapped inside.Impact
Signers can easily collude to freeze the contract, giving themselves the power to hold the protocol ransom to unfreeze the safe and all funds inside it.
In a less malicious case, signers might accidentally add too many owners and end up needing to manage the logistics of having users renounce their hats.
Code Snippet
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L507-L529
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L187-L189
Tool used
Manual Review
Recommendation
There should be a check in
checkAfterExecution()
that ensures that the number of owners on the safe has not changed throughout the execution.It also may be recommended that the
maxSigners
value is adjustable by the contract owner.