Skip to content

Latest commit

 

History

History
43 lines (30 loc) · 1.85 KB

005.md

File metadata and controls

43 lines (30 loc) · 1.85 KB
Raw

Formal Peach Starling

Medium

Users can bypass intended restrictions and add modules to the Safe

Summary

If the call intended to be executed is a delegatecall, the contract's checkTransaction stores the current list of owners. Then in the checkAfterExecution it verifies that the list has not changed. The checkAfterExecution function also checks that no modules have been added.

    if (operation == Enum.Operation.DelegateCall) {
      // case: DELEGATECALL
      // We disallow delegatecalls to unapproved targets
      if (!enabledDelegatecallTargets[to]) revert DelegatecallTargetNotEnabled();

      // Otherwise record the existing owners and threshold for post-flight checks to ensure that Safe state has not
      // been altered
      _existingOwnersHash = keccak256(abi.encode(owners));
      _existingThreshold = threshold;
      _existingFallbackHandler = safe.getSafeFallbackHandler();

However, both of these assume that the owner or module to be added is added to the regular functions, which uses an array as a sorted list. The delegatecall however allows to simply bypass the intended functions and change the mapping values in such way that an address is given owner rights without actually being in that ordered list.

This renders the following checks useless.

Affected Code

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

Attack Path

  1. Signers do a delegatecall
  2. Said delegatecall adds a module by simply setting the storage value modules[moduleAddr] = randomAddr
  3. This will bypass the checkAfterExecution checks and renders them useless.

Also breaks the following invariant which should never be broken

There should never be more than 1 module enabled on the safe

Impact

Users can bypass intended restrictions

Mitigation

Fix is non-trivial.