Allow Safe to Own Itself #998
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR reverts an old change (See #229 and #259) and allows the Safe to once again own itself. This is a step towards 7702 support, whereby we want the EOA that delegates to a Safe to be able to sign transactions for itself. The restriction was originally put in place because `execTransaction` would call `isValidSignature` (which used to be implemented on the Safe itself and not as part of the compatibility fallback handler) on itself, where the `msg.sender` would be the Safe itself, and allow for trivial "approved hash" signatures (through [this](https://github.com/safe-global/safe-smart-account/blob/36db12a140331c522e773849ebc87d39f5abae1e/contracts/GnosisSafe.sol#L244-L245) mechanism). This is no longer possible since v1.3.0, as the `isValidSignature` call would be handled by the fallback handler, meaning that the `msg.sender` that can trivially sign "approve hash" signatures is no longer the Safe itself but the configured fallback handler. Since #866 and starting in v1.5.0 the `CompatibilityFallbackHandler` is implemented to disallow "approve hash" signatures regardless of the caller, further eliminating the requirement for the restriction. ### Increasing Threshold Concerns One concern with this change, and removing the restriction on having the Safe own itself, is that the Safe cannot sign for itself, so you can potentially block access to the account completely, by requiring the Safe "self-owner" to be included as part of the signing threshold. For example, if you have a n/n Safe (so `threshold == ownerCount` and one of the owners is the Safe itself, then the Safe will not be able to ever produce a valid signature).
nlordell
commented
Jun 20, 2025
| @@ -165,7 +154,7 @@ describe("Safe", () => { | |||
| } = await setupTests(); | |||
| await expect( | |||
| template.setup([user2.address, user2.address], 2, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero), | |||
| ).to.be.revertedWith("GS203"); | |||
| ).to.be.revertedWith("GS204"); | |||
There was a problem hiding this comment.
The error duplicate owner is now consistently "GS204".
nlordell
commented
Jun 20, 2025
| } as const; | ||
| const signatures = buildSignatureBytes([selfSignature]); | ||
|
|
||
| await expect(safe["checkSignatures(address,bytes32,bytes)"](user1.address, txHash, signatures)).to.be.revertedWith("GS025"); |
There was a problem hiding this comment.
Note that if we change the CompatibilityFallbackHandler to not use address(0) as the executor, then this test would fail:
diff --git a/contracts/handler/CompatibilityFallbackHandler.sol b/contracts/handler/CompatibilityFallbackHandler.sol
index 275797d..e652ad0 100644
--- a/contracts/handler/CompatibilityFallbackHandler.sol
+++ b/contracts/handler/CompatibilityFallbackHandler.sol
@@ -80,7 +80,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
} else {
// We explicitly do not allow caller approved signatures for EIP-1271 to prevent unexpected behaviour. This
// is done by setting the executor address to `0` which can never be an owner of the Safe.
- safe.checkSignatures(address(0), messageHash, _signature);
+ safe.checkSignatures(msg.sender, messageHash, _signature);
}
return EIP1271_MAGIC_VALUE;
} 1) Safe
checkSignatures
should not allow trivial signatures when the Safe owns itself:
AssertionError: Expected transaction to be reverted with reason 'GS025', but it didn't revert
at async Context.<anonymous> (test/core/Safe.Signatures.spec.ts:511:13)
|
Tagging with |
It is no longer true, and is allowed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change was originally proposed by @akshay-ap
This PR reverts an old change (See #229 and #259) and allows the Safe to once again own itself. This is a step towards 7702 support (#997), whereby we want the EOA that delegates to a Safe to be able to sign transactions for itself.
The restriction was originally put in place because
execTransactionwould callisValidSignatureon itself (which used to be implemented on the Safe itself and not as part of the compatibility fallback handler), where themsg.senderwould be the Safe itself, and allow for trivial "approved hash" signatures (throughthis mechanism).
This is no longer possible since v1.3.0, as the
isValidSignaturecall would be handled by the fallback handler, meaning that themsg.senderthat can trivially sign "approve hash" signatures is no longer the Safe itself but the configured fallback handler. Since #866 and starting in v1.5.0 theCompatibilityFallbackHandleris implemented to disallow "approve hash" signatures regardless of the caller, further eliminating the requirement for the restriction.Increasing Threshold Concerns
One concern with this change, and removing the restriction on having the Safe own itself, is that the Safe cannot sign for itself, so you can potentially block access to the account completely, by requiring the Safe "self-owner" to be included as part of the signing threshold. For example, if you have a n/n Safe (so
threshold == ownerCount) and one of the owners is the Safe itself, then the Safe will not be able to ever produce a valid signature.