Merry Marigold Ferret - Threshold of existing Safe, could cause transaction failures

[sherlock-admin2]

Merry Marigold Ferret

Medium

Threshold of existing Safe, could cause transaction failures

Summary

When creating a new HSG module it is possible to attach it to an existing Safe. The existing Safe can have an already set up owners and threshold. However, even though the owners receive the required by the HSG module hats, the transactions will not succeed.

Root Cause

The root cause of this issue is due to the unchecked number of Safe's owners in the initializer of the HSG module. Every other change whether of the owners(ref), or the ThresholdConfig(ref), the number of owners are checked against the boundaries described in the the ThresholdConfig and the Safe's threshold is updated according to that value. In checkTransaction(), the Safe's threshold is expected to be equal to the result of _getRequiredValidSignatures.

However when a new HSG module is attached to an existing Safe, or the Safe is migrated to a new HSG, the Safe's threshold is not updated to match the boundaries in the ThresholdConfig. Which will lead to failures in checkTransaction(), because the Safe's threshold is expected to be equal to the result of the _getRequiredValidSignatures.

Internal pre-conditions

1. A HSG's ThresholdConfig should be ABSOLUTE, and target less than the number of owners of the Safe.

External pre-conditions

1. A Safe is created with threshold equal to the number of owners.
2. All owners receive valid signer's hats.

Attack Path

1. A HSG module is created and added to a Safe.
2. All owners of the Safe call claimSigner()
3. A transaction is executed using the Safe's executeTransaction(), which contains Safe's threshold number of valid signatures.
4. The checkTransaction() will fail.

Impact

When attaching a HSG module to an existing Safe, all transactions will revert due to the difference of the Safe's threshold and the module's target.

PoC

Let's assume that a Safe is created with 10 owners and a threshold of 5.

A HSG module is attached to it with a ThresholdConfig, with min=2 and target=7.

The Safe's threshold is in the boundaries of the HSG.

When a transaction is executed from the Safe, the checkTransaction is called.

This function fetches the Safe's threshold and owners.

After that it calculates the threshold which is required by the HSG module, using the number of owners.
In the given example it will require 7, because the number of owners is bigger than the module's target.

However the Safe's threshold i equal to 5, so the following check will fail:

Ref

if (threshold != _getRequiredValidSignatures(owners.length)) revert ThresholdTooLow();

Leading to the failure of the whole transaction.

Mitigation

Add checks and threshold updates like this:

    // update the safe's threshold to match the new config
    address[] memory owners = safe.getOwners();
    // get the required amount of valid signatures according to the new threshold config
    // and the current number of owners
    uint256 newThreshold = _getRequiredValidSignatures(owners.length);
    // the safe's threshold cannot be higher than the number of owners (safe's invariant)
    if (newThreshold > owners.length) {
      newThreshold = owners.length;
    }

    safe.execChangeThreshold(newThreshold);

when a HSG module is attached to a Safe.