Docile Yellow Dachshund - Old signers and HSG can remove new signers and HSG, hijacking the safe wallet

[sherlock-admin4]

Docile Yellow Dachshund

Medium

Old signers and HSG can remove new signers and HSG, hijacking the safe wallet

Summary

When the DAO decides to migrate from one HSG to another along with its signers, it will call the HatsSignerGate::migrateToNewHSG function. This will remove the current HSG as a guard and module from the safe and add the new signers.

https://github.com/sherlock-audit/2024-11-hats-protocol/blob/49de29508904e95b3cfaaf27d2e76c527429c019/hats-zodiac/src/HatsSignerGate.sol#L353

At first this won't be an issue, as the new HSG is set as the module and guard. But we don't see the removal of the old HSG and old signers as owners from the safe wallet. This has a potential to create an issue.

If the threshold is absolute and is not updated before the migration of HSG, then the old HSG and signers (owners) can collude together to execute any transaction since they are not removed and the old threshold is enough for them to execute transactions. They can possibly remove the new HSG or any signer(s) they want.

I believe they have no reason to act honestly after they are migrated. So collusion is a valid concern.

Root Cause

The root cause is inside HatsSignerGate::migrateToNewHSG method as it does not remove the old HSG and signers in the same transaction. It is important to be called in the same transaction as later ones can be front-runned and the malicious HSG and signers can remove the new HSG or signers before.

This is possible assuming that the old signers (including HSG) are >= threshold. If the threshold is absolute then it won't get changed unless the transaction to update the target is initiated.

Internal pre-conditions

1. The threshold is ABSOLUTE

External pre-conditions

No response

Attack Path

The attack path is simple. Just front-run every possible transaction that removes the old HSG, old signers and updates the threshold until they have removed the new ones.

Impact

Malicious signers will take control over the safe wallet posing the users funds and rights at risk.

PoC

No response

Mitigation

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);

  //   // removing old signers
    address[] memory owners = safe.getOwners();
    for (uint i; i<owners.length; i++) {
      removeSigner(owners[i]);
    }
    // remove the old HSG
    removeSigner(address(this));

  //   // 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);
  // }