Skip to content

Latest commit

 

History

History
60 lines (44 loc) · 2.35 KB

015.md

File metadata and controls

60 lines (44 loc) · 2.35 KB
Raw

Formal Peach Starling

High

migrateToNewHSG accidentally removes the new HSG as a module

Summary

HSG v2 allows users to migrate their Safe to a potential new version of the HSG, using the migrateToNewHSG function

  function migrateToNewHSG(address _newHSG, uint256[] calldata _signerHatIds, address[] calldata _signersToMigrate)
    public
  {
    _checkUnlocked();
    _checkOwner();

    ISafe s = safe; // save SLOADS
    // remove existing HSG as guard
    s.execRemoveHSGAsGuard();
    // enable new HSG as module and guard
    s.execAttachNewHSG(_newHSG);
    // remove existing HSG as module
    s.execDisableHSGAsModule(_newHSG);   // @audit -> problem 

    // if _signersToMigrate is provided, migrate them to the new HSG by calling claimSignersFor()
    uint256 toMigrateCount = _signersToMigrate.length;
    if (toMigrateCount > 0) {
      // check that the arrays are the same length
      if (_signerHatIds.length != toMigrateCount) revert InvalidArrayLength();

      IHatsSignerGate(_newHSG).claimSignersFor(_signerHatIds, _signersToMigrate);
    }
    emit Migrated(_newHSG);
  }

However, as we can see it accidentally calls execDisableHSGAsModule is called with _newHSG which just disables the new HSG, instead of the old one. This leaves the old HSG as the connected module and breaks operability as the new HSG is set to being a guard. As the new HSG is not a module, migrating back to the old HSG will not be possible.

Executing transactions would still be possible, but changing crucial parameters such as threshold will not be possible. For the same reason, removing signers will not be possible.

Root Cause

Calling function with wrong argument.

Affected Code

https://github.com/sherlock-audit/2024-11-hats-protocol/blob/main/hats-zodiac/src/HatsSignerGate.sol#L365

Attack Path

  1. DAO wishes to migrate to HSG v3
  2. DAO calls migrateToNewHSG
  3. Due to faulty logic, old HSG remains the protocol's module
  4. New HSG implementation can no longer change its threshold.
  5. If for some reason DAO cannot reach threshold, all funds are permanently lost (e.g. due to requiring X/X signatures and one of signers going rogue)

Impact

Broken functionality. Unable to change threshold and overall execute any Safe transaction from Module. Possibly full loss of funds.

Mitigation

Call the function with the correct argument.