Warning for mismatching mastercopy in safe proxy #3908

schmanu · 2022-05-24T16:49:18Z

What it solves

Resolves #3873

How this PR fixes it

Displays a warning in the SafeHeader if the safe uses a invalid master copy contract.

How to test it

Open this safe which was deployed with an invalid master copy
matic:0x5B7e9a88237Ca67591e751186b10623Db5653045

Screenshots

Cases to check:

all supported versions on L1 and L2 networks should not have error message
safes with 1.3.0 mastercopy on L2 networks - arb1:0x302FFc62219e70Bd578Fc88C1ba0922511C8151f (Qa test safe with 1.3.0 mastercopy on Arbitrum instead of 1.3.0+L2)
safes with 1.3.0 +L2 mastercopies on L1 networks
wrong mastercopies ( deploy safe on L2 with 1.1.1 mastercopy or folk mastercopy - BSC had unsupported master copies)

github-actions · 2022-05-24T16:49:31Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-05-24T16:52:20Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	1	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-05-24T16:58:11Z

Deployment links

⚪ Mainnet 🟣 Polygon 🟠 Rinkeby

coveralls · 2022-05-24T16:58:45Z

Pull Request Test Coverage Report for Build 2514574842

27 of 30 (90.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 36.956%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/logic/safe/store/reducer/safe.ts	0	1	0.0%
src/components/AppLayout/InvalidMasterCopyError/index.tsx	15	17	88.24%

Totals
Change from base Build 2513979434:	0.1%
Covered Lines:	3884
Relevant Lines:	9557

💛 - Coveralls

src/components/AppLayout/Sidebar/SafeHeader/index.tsx

src/logic/safe/utils/safeVersion.ts

DiogoSoaress

Nice! I've also the solution and I think it is important to ship a quick fix for it. Nevertheless we should iterate on this issue with a more thoughtful design in mind. wdyt?

One visual remark:
Can we align this icon vertically? It is a bit off

src/components/AppLayout/Sidebar/SafeHeader/index.tsx

src/logic/safe/utils/safeVersion.ts

liliya-soroka · 2022-05-25T09:31:11Z

Note : the error is shown for the valid mastercopy 1.1.1 - safe for testing rin:0x43852dB7B5ACE1e57d853789D331caeD88Af3cFF
Take into account that we are supporting 1.0.0, 1.1.1, 1.2.0 on rinkleby, Mainnet, xDai

* refactor master copy validation to reuse safeContracts getSafeContractDeployment * change test + new testcase

schmanu · 2022-05-25T12:29:17Z

Note : the error is shown for the valid mastercopy 1.1.1 - safe for testing rin:0x43852dB7B5ACE1e57d853789D331caeD88Af3cFF Take into account that we are supporting 1.0.0, 1.1.1, 1.2.0 on rinkleby, Mainnet, xDai

I found the issue and fixed it. I also added a new testcase for this.

katspaugh · 2022-05-25T13:20:32Z

src/components/AppLayout/InvalidMasterCopyError/index.tsx

+        This safe is using an invalid master copy contract. The web interface might not work correctly. It&apos;s
+        recommended to use the{' '}


Suggested change

This safe is using an invalid master copy contract. The web interface might not work correctly. It's

recommended to use the{' '}

This Safe is created with an unsupported base contract. The web interface might not work correctly. We recommend using the{' '}

The ecosystem term for the "base" contract is an implementation contract. But in our codebase we use "singleton", I'd suggest to keep the wording aligned

Thanks for the correction Mikhail. IMHO implementation contract is clear, singleton sounds like too much detail.
Let's just drop the word base maybe?
I.e. This Safe was created with an unsupported contract

This Safe was created with an unsupported contract

It may be too broad, I feel like creating a Safe with an unsupported factory contract also can be described like this

Yes, and that would also warrant this banner.

iamacook

Looks good! There are a few linting issues re return types.

iamacook · 2022-05-26T09:47:25Z

src/logic/safe/utils/__tests__/safeVersion.test.ts

@@ -36,4 +40,50 @@ describe('Check safe version', () => {
      expect(hasFeature(FEATURES.SAFE_APPS, '1.1.1')).toBe(true)
    })
  })
+
+  describe('isValidMasterCopy', () => {


I think we should add some more tests so that we cover all deployed versions.

Fixed the linter and prettier warnings and added tests for 1.0.0 and 1.2.0!

src/components/AppLayout/InvalidMasterCopyError/index.tsx

katspaugh · 2022-05-27T06:46:02Z

We're having quite a few errors on Sentry from Safes like this btw:
https://safe-client.gnosis.io/v1/chains/56/safes/0xDF951b3657777ee21a3B01494C362d2ea9d4378E

It's from a Safe deployed on an unsupported BSC fork of the contract.

It would be nice to also handle cases like that. As you can see in the Safe Info payload, the version is UNKNOWN.

But otherwise I think it's ready for QA.

Co-authored-by: Aaron Cook <[email protected]>

Dismissing because that button isn't there anymore.

liliya-soroka · 2022-05-27T12:51:54Z

5

@katspaugh , I would expect that such case is covered on the add safe flow as not supported mastercopy on address detection - should we report it as a separate issue ?

francovenica · 2022-05-27T15:53:04Z

I've found this specific safe, one I created a while ago using the safe UI. This safe shows the banner and I don't think it should

https://pr3908--safereact.review-safe.gnosisdev.com/app/gno:0x157C6adB6786fB471C9969cAc2D77e09c4e2599d/transactions/history

https://safe-client.gnosis.io/v1/chains/100/safes/0x157C6adB6786fB471C9969cAc2D77e09c4e2599d

EDIT: I tried a tx (owner removal) and it worked just fine

francovenica · 2022-05-27T15:56:34Z

Another:
https://pr3908--safereact.review-safe.gnosisdev.com/app/matic:0x6fB53537015FA77Dd0132D6532A1A0d392e67A99/transactions/history

https://safe-client.gnosis.io/v1/chains/137/safes/0x6fB53537015FA77Dd0132D6532A1A0d392e67A99

schmanu · 2022-05-29T16:59:58Z

@francovenica I checked the safes which were flagged as invalid and I think they use the L1 master copy instead of the L2 master copy:

Here is a IMO correct safe using the L2 master copy on matic:
https://pr3908--safereact.review-safe.gnosisdev.com/app/matic:0x4aA4981f187550AbcA155C4b45C05F73b4625760/balances
https://safe-client.gnosis.io/v1/chains/137/safes/0x4aA4981f187550AbcA155C4b45C05F73b4625760
I does not show the banner and the implementation value for the safe info endpoint returns

"implementation":{
  "value":"0x3E5c63644E683549055b9Be8653de26E0B4CD36E",
  "name":"Gnosis Safe: Singleton L2 1.3.0",
  "logoUri":"https://safe-transaction-assets.gnosis-safe.io/contracts/logos/0x3E5c63644E683549055b9Be8653de26E0B4CD36E.png"}

While both safes you provided return

"implementation":{
  "value":"0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552",
  "name":"Gnosis Safe: Singleton 1.3.0",
  "logoUri":"https://safe-transaction-assets.gnosis-safe.io/contracts/logos/0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552.png"}

These are the safe-deployments for L2 and version 1.3.0: https://github.com/safe-global/safe-deployments/blob/main/src/assets/v1.3.0/gnosis_safe_l2.json
For Gnosis chain:
"100": "0x3E5c63644E683549055b9Be8653de26E0B4CD36E"
And polygon
"137": "0x3E5c63644E683549055b9Be8653de26E0B4CD36E"
which both don't match the address returned for the safes you provided.

This makes me wonder if we miss a way of creating invalid safes if you used the UI for creating them or if I misunderstand how to get the expected version to compare it with the safe info.

rmeissner · 2022-05-29T18:49:18Z

src/logic/safe/utils/__tests__/safeVersion.test.ts

+      expect(isValid).toBe(true)
+    })
+
+    it('returns true for L1 mastercopy in L2 chain and version 1.0.0', async () => {


What about an L2 version on the L1 chain?

Also a more general note: if a specific master copy is supported depends on the service running it. E.g. mainnet and GC support all master copies while all other networks only support the 1.3.0+L2 version. Not sure what the expected behavior is for this PR, but if the goal is to show what version can be used with our services, we should probably rely on server side information.

Is there an endpoint to get the server side information which versions are supported by our services for a chainID?
I agree that it would be the cleanest solution.

I believe it's this one: https://safe-transaction.optimism.gnosis.io/api/v1/about/master-copies/, but it needs to be extended to flag non-indexed singletons

There's a gateway endpoint too: /v1/chains/{chainId}/about/master-copies. The SDK has master copy function. It too would also need to be extended.

Hmm could we probably detect it when looking at the lastIndexedBlockNumber?

For some L2 networks where the L1 master copy won't work like polygon it seems to stay the same as the deployedBlockNumber: https://safe-transaction.polygon.gnosis.io/api/v1/about/master-copies/

I thought about that too, but not sure how reliable it is. I'd ask the infra team

Apparently the infra team was considering to remove all non-relevant / unsupported master copies and will do that soon.
So I will change the PR now to use this endpoint and simply check if the master copy version is included in the response.

liliiaorlenko · 2022-06-07T09:51:14Z

in a matter of design:

could u pls check the alignment of the text and icons (now text is a bit more up then icons)
align info icon and text to the left, close icon to the right, thats a style of our error messages

The infra team will change the endpoint such that only valid master copies will be returned by the sdk.

schmanu · 2022-06-09T08:37:55Z

This PR should work as soon as safe-global/safe-transaction-service#887 is live!

src/logic/safe/utils/__tests__/safeVersion.test.ts

francovenica · 2022-06-14T04:37:52Z

I have times where a safe sometimes fails the initial load and then loads properly, something that is not rare in some networks. The thing is that when it fails to load it shows the warning message, and then when it loads properly the message still shows. If you refresh the page the message is gone.

I'm not sure if the system can check if the safe just failed to load so it doesn't show the message until it verifies that is the MC is actually not compatible.
https://pr3908--safereact.review-safe.gnosisdev.com/app/arb1:0xc700e31c608b97Dc4FdbD49B7f921626c1403644/home

francovenica · 2022-06-14T04:43:10Z

Even with the comment I made before, the ticket looks fine and I'm ok with passing it to done

I just checked that for all the safe with a proper MC version, (those who show the safe version in the settings) don't show the warning message and those safe with an incompatible MC version show the warning message.
I haven't found an instance where this doesn't happen (safes with a compatible version of MC with the warning message or safes with incompatible MC versions w/o the warning message).

Is impossible to be sure that it works for every safe out there, but I've checked over 100 safes in different networks and I didn't had issues

francovenica · 2022-06-16T01:17:45Z

This ticket was put in QA, but I don't see anything new.
A suggestion I can make is that, if the safe reloads on its own after failing to load the first time, the verification if the MC is compatible can be done again, and the warning message should disappear once the safe loads properly. That would be fine

github-actions · 2022-06-16T12:08:51Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	4	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/components/AppLayout/InvalidMasterCopyError/index.tsx

iamacook · 2022-06-16T15:24:49Z

src/components/AppLayout/Sidebar/SafeHeader/index.tsx

@@ -233,7 +236,6 @@ const SafeHeader = ({
      </Container>
    )
  }
-  const chainInfo = getChainInfo()


Please avoid making unnecessary changes like this.

iamacook · 2022-06-16T15:25:51Z

src/components/AppLayout/index.tsx

@@ -134,6 +136,7 @@ const Layout: React.FC<Props> = ({
 }): React.ReactElement => {
  const [mobileNotSupportedClosed, setMobileNotSupportedClosed] = useState(false)
  const [expanded, setExpanded] = useState(false)
+  const [showMasterCopyError, setShowMasterCopyError] = useState(true)


If you return null from InvalidMasterCopyError when it's valid then you no longer need this state.

I think this state is for when the user closes the banner

This is used to hide the error if the user closes the alert.
I moved this state into the InvalidMasterCopyError component as it is not needed outside of it.

src/components/AppLayout/InvalidMasterCopyError/index.tsx

src/logic/safe/store/reducer/safe.ts

Co-authored-by: Aaron Cook <[email protected]>

…ning

francovenica · 2022-06-21T08:41:48Z

Ok, I'm not getting the warning message if the safe fails to load.

Looks good to me

schmanu added 3 commits May 24, 2022 18:06

fix: show error when detecting invalid master copy address

dfa4e1f

refactor and test new master copy validation

2798f6a

fix: small error in rendering

ec59423

schmanu self-assigned this May 24, 2022

katspaugh reviewed May 25, 2022

View reviewed changes

src/components/AppLayout/Sidebar/SafeHeader/index.tsx Outdated Show resolved Hide resolved

src/components/AppLayout/Sidebar/SafeHeader/index.tsx Outdated Show resolved Hide resolved

src/components/AppLayout/Sidebar/SafeHeader/index.tsx Outdated Show resolved Hide resolved

usame-algan reviewed May 25, 2022

View reviewed changes

src/logic/safe/utils/safeVersion.ts Outdated Show resolved Hide resolved

DiogoSoaress previously requested changes May 25, 2022

View reviewed changes

src/components/AppLayout/Sidebar/SafeHeader/index.tsx Outdated Show resolved Hide resolved

src/logic/safe/utils/safeVersion.ts Outdated Show resolved Hide resolved

change: use topbar alert instead of tooltip

ac7b9df

* refactor master copy validation to reuse safeContracts getSafeContractDeployment * change test + new testcase

katspaugh reviewed May 25, 2022

View reviewed changes

schmanu added 2 commits May 25, 2022 15:33

reword error

e5127d3

fix: reword error message

8016673

iamacook reviewed May 26, 2022

View reviewed changes

Update src/components/AppLayout/InvalidMasterCopyError/index.tsx

55c2360

Co-authored-by: Aaron Cook <[email protected]>

chore: add more tests, fix prettier and lint warnings

7809713

rmeissner reviewed May 29, 2022

View reviewed changes

fix: layout of error message, add test case

2233a1a

DiogoSoaress mentioned this pull request Jun 8, 2022

Load safe form - Safe validation with unsupported mastercopies #3923

Closed

change: determines available valid master copies from gateway sdk

32f2c9f

The infra team will change the endpoint such that only valid master copies will be returned by the sdk.

DiogoSoaress approved these changes Jun 9, 2022

View reviewed changes

DiogoSoaress reviewed Jun 9, 2022

View reviewed changes

src/logic/safe/utils/__tests__/safeVersion.test.ts Show resolved Hide resolved

DiogoSoaress reviewed Jun 9, 2022

View reviewed changes

src/logic/safe/utils/__tests__/safeVersion.test.ts Outdated Show resolved Hide resolved

fix: Unused import

b428720

fix: Store safe implementation object and use for master copy check

a448216

usame-algan and others added 2 commits June 16, 2022 16:09

chore: Remove console logs

ad75c7e

fix: unit test for isValidMasterCopy

693f860

schmanu force-pushed the invalid-mastercopy-warning branch from 9578056 to 693f860 Compare June 16, 2022 14:30

usame-algan and others added 2 commits June 16, 2022 17:10

chore: Test InvalidMasterCopyError component

3c9d5b8

fix: unit test for safeInformation

3063732

usame-algan requested a review from iamacook June 16, 2022 15:14

iamacook reviewed Jun 16, 2022

View reviewed changes

Manuel Gellfart and others added 3 commits June 16, 2022 17:36

change: noopener & noreferrer for CLI Link

20c7abe

Co-authored-by: Aaron Cook <[email protected]>

fix: move close-state into InvalidMasterCopyError, new testcase, comment

4f466ef

Merge remote-tracking branch 'origin/dev' into invalid-mastercopy-war…

2111359

…ning

usame-algan requested a review from iamacook June 17, 2022 09:20

iamacook approved these changes Jun 17, 2022

View reviewed changes

schmanu merged commit cd9d905 into dev Jun 21, 2022

schmanu deleted the invalid-mastercopy-warning branch June 21, 2022 10:18

github-actions bot locked and limited conversation to collaborators Jun 21, 2022

		This safe is using an invalid master copy contract. The web interface might not work correctly. It's
		recommended to use the{' '}

	This safe is using an invalid master copy contract. The web interface might not work correctly. It's
	recommended to use the{' '}
	This Safe is created with an unsupported base contract. The web interface might not work correctly. We recommend using the{' '}

Warning for mismatching mastercopy in safe proxy #3908

Warning for mismatching mastercopy in safe proxy #3908

Conversation

schmanu commented May 24, 2022 • edited by liliya-soroka Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented May 24, 2022 • edited Loading

github-actions bot commented May 24, 2022 • edited Loading

ESLint Summary View Full Report

github-actions bot commented May 24, 2022

Deployment links

coveralls commented May 24, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2514574842

💛 - Coveralls

DiogoSoaress left a comment

Choose a reason for hiding this comment

liliya-soroka commented May 25, 2022

schmanu commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh commented May 27, 2022 • edited Loading

liliya-soroka commented May 27, 2022

francovenica commented May 27, 2022 • edited Loading

francovenica commented May 27, 2022

schmanu commented May 29, 2022

rmeissner May 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liliiaorlenko commented Jun 7, 2022

schmanu commented Jun 9, 2022

francovenica commented Jun 14, 2022 • edited Loading

francovenica commented Jun 14, 2022

francovenica commented Jun 16, 2022

github-actions bot commented Jun 16, 2022 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francovenica commented Jun 21, 2022

schmanu commented May 24, 2022 •

edited by liliya-soroka

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

github-actions bot commented May 24, 2022 •

edited

Loading

coveralls commented May 24, 2022 •

edited

Loading

katspaugh commented May 27, 2022 •

edited

Loading

francovenica commented May 27, 2022 •

edited

Loading

rmeissner May 29, 2022 •

edited

Loading

francovenica commented Jun 14, 2022 •

edited

Loading

github-actions bot commented Jun 16, 2022 •

edited

Loading