Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way to identify whether a contract is a SAFE. #714

Open
MicahZoltu opened this issue Dec 12, 2023 · 7 comments
Open

Provide a way to identify whether a contract is a SAFE. #714

MicahZoltu opened this issue Dec 12, 2023 · 7 comments
Labels
future Features for next major contract version

Comments

@MicahZoltu
Copy link
Contributor

Context / issue

When building applications that interact with SAFEs, it is necessary to first validate that a given address (e.g., user supplied or on-chain scanned) is actually a SAFE (or at least claims to be). Currently, the only way to do this is to either hard-code all versions of Gnosis SAFE proxy and master contracts and check to see if the contract is a proxy and if it points to a known master copy. Unfortunately, this doesn't scale in a censorship resistant way because as new SAFEs are developed, existing immutable applications now need to be updated with new SAFE proxies/masters. This solution also doesn't work if someone uses a non-standard proxy or if they deploy their own master directly.

Proposed solution

A. Add a getter on the SAFE master that just returns a constant string like Gnosis SAFE, along with perhaps a method that returns the VERSION constant. This would allow applications to simply call that function and see what it returns.

B. A more ecosystem friendly solution would be to define ERC-165 interfaces for all of the different components of a SAFE so users can query to see if a particular interface is supported. For example, the various module add/get/remove functions could be part of one interface, and the owners/threshold could be another interface, and the execution functions could be a third interface. This would allow an application to check to see if the set of methods it needs are supported by any contract (SAFE or not) and then interact with the contract regardless of whether it is a Gnosis SAFE or something else.

@MissLov3ly
Copy link

IM honestly trying so hard to keep up with how fast the development moves alongm with this project..
I invested early and didnt have a clue what I had until it was already so established that I have yet to catcth up!
Its been two years...honestly I could use some pointers if you have time from your busy schedule as I know time is so valuable.
I REALLY NEED HELP =(

@nlordell
Copy link
Collaborator

nlordell commented Nov 1, 2024

I'm going to close this for now as a "won't fix".

We could add something like a SAFE_ACCOUNT_IDENTIFIER method (or something similarly named) that returns Safe v1.5.0 and Safe v1.5.0+L2. However, I'm not convinced this is a good idea - as it isn't a reliable method to check that a contract is in fact a Safe (any contract can "spoof" this), so it would not provide a reliable way to identify an address as a Safe anyway and there are some technical gotchas associated with using this information (i.e. integrators may mistakenly believe it is a reliable way to determine an account is a Safe, when it actually isn't).

In fact, we used to have a NAME in earlier versions that was removed as it was deemed that it did not add sufficient value, and to save on code size (something that is is short supply for the Safe contract):

https://github.com/safe-global/safe-smart-account/blob/v1.1.1/contracts/GnosisSafe.sol#L20

Additionally, the Safe account already has some pretty good methods for guessing whether or not a contract is a Safe:

  • You could use the existing VERSION() to check it matches an expected version
  • You could use domainSeparator() function to compute and return the EIP-712 domain separator used for various signatures in the Safe. You can calculate a value locally and check that they match.

However, if you want to reliably determine an account is actually a Safe onchain, I don't think there is a solution that does not envolve validating the EXTCODEHASH matches a well-known proxy code hash, and that the EXTCODEHASH of the masterCopy() matches a well-known singleton code hash (using EXTCODEHASH would allow alternative deployments to work as well).

@mmv08
Copy link
Member

mmv08 commented Nov 1, 2024

However, if you want to reliably determine an account is actually a Safe onchain, I don't think there is a solution that does not envolve validating the EXTCODEHASH matches a well-known proxy code hash, and that the EXTCODEHASH of the masterCopy() matches a well-known singleton code hash (using EXTCODEHASH would allow alternative deployments to work as well).

Here's the example of Argent wallet contract doing exactly this: https://github.com/argentlabs/argent-contracts/blob/develop/contracts/infrastructure/ArgentWalletDetector.sol

@MicahZoltu
Copy link
Contributor Author

I don't believe that this issue should be closed for the reasons you have listed. The feature you are describing in your closing message is not the feature I am requesting here.

I am not looking for a way to tell with certainty that a contract is a SAFE. I'm looking for a way to ask the contract if it claims to be a SAFE. This is for integration with tooling so the tooling can know what calls it can reasonably make to the contract. The solution does not need to be robust against people spoofing being a SAFE anymore than a website needs to be resilient to people spoofing their browser. When a tool asks a contract whether or not it is a SAFE, the contract can lie and it just means that the tool will present interaction options that don't actually work. This is very similar to things like EIP-165 where a contract can, if it wanted, lie about what methods are available but all that does is makes it so users may call non-existent methods on the contract.

In our specific use case, we want a way to tell users whether the address they have selected is an EOA, a contract wallet, or an app contract. It gives the user useful information and hints how we present the address in our UI. It also lets us know which list to put it in and what UI to present to the user when they are interacting with the address.

VERSION() doesn't tell us anything about the interface of the address, and is likely to conflict between contracts.

domainSeparator may work, I'll need to investigate this. Is domain separator the same for every version of SAFE wallets?

EXTCODEHASH doesn't work because it requires maintaining a centralized database of all known SAFE hashes, all known proxies, and doesn't provide a path to third parties using their own SAFE-like wallets. It also is far more robust than we actually want. We want people to be able to spoof being a SAFE if that is desired, just like people used to spoof being Internet Explorer or spoof being MetaMask for compatibility.

EIP-165 would be an ideal solution as I mentioned, and I would like to see that implemented in future versions of SAFE.

@nlordell
Copy link
Collaborator

nlordell commented Nov 4, 2024

domainSeparator may work, I'll need to investigate this. Is domain separator the same for every version of SAFE wallets?

It is (as in, it hasn't changed in Safe versions), but the domain separator is specific to each Safe.

EIP-165 would be an ideal solution as I mentioned, and I would like to see that implemented in future versions of SAFE.

Makes sense. This requires a bit of special handling, however, in order to interact correctly with fallback handlers. One thing that we can do for now, is to change the existing supportsInterface implementation in the fallback handler to report that it supports the type(ISafe).interfaceId:

function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
return
interfaceId == type(ERC1155TokenReceiver).interfaceId ||
interfaceId == type(ERC721TokenReceiver).interfaceId ||
interfaceId == type(IERC165).interfaceId;
}

@nlordell nlordell reopened this Nov 4, 2024
@mmv08
Copy link
Member

mmv08 commented Nov 4, 2024

It is (as in, it hasn't changed in Safe versions), but the domain separator is specific to each Safe.

In version 1.1.1, the domain separator didn't include the chain id. Starting from 1.3.0, it should be the same.

@MicahZoltu
Copy link
Contributor Author

supportsInterface returning true for ISafe`` I think would be good. It would inform me as an end-user tool author how I should present the address to the user (as an EOA vs app contract vs contract wallet). Ideally, future SAFE's would return true for that check, even if ISafegains new methods. One way to achieve this would be to have some basic interface that is added to via child classes likeISafe2 extends ISafe1or something. Then in v2 of the interfacesupportsInterfacewould return true for bothinterfaceId == type(ISafe1).interfaceIdandinterfaceId == type(ISafe2).interfaceId` so any apps built to work with v1 would still correctly recognize v2 safe's.

Of course, this strategy stops working if you ever need to remove or change a function from the interface. How stable do you expect SAFE's to be over time? One solution is to create very small interfaces that are unlikely to change, but even the execute function (the core of a SAFE arguably) may be subject to changes in the future.

Thanks for reopening!

@nlordell nlordell added future Features for next major contract version and removed wontfix labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future Features for next major contract version
Projects
None yet
Development

No branches or pull requests

4 participants