Add API-level proxy detection and implementation resolution #1711

manuelwedler · 2024-10-22T13:41:20Z

This implements an API-level proxy detection as in #1655 by making use of the whatsabi library. A few explanations about this PR:

I had to add two methods to the SourcifyChain class for making it compatible to the whatsabi library as a provider, and for being able to resolve the implementation addresses of DiamondProxy contracts.
I added a proxyResolutionError to the API response for the case that the proxy resolution fails. This could be due to an unresponsive RPC for example. In such a case, I still want to respond successfully to the user but let them know why the proxy fields are not there.
Related to proxies: Multiple instances of the same resolver shazow/whatsabi#142:
- A FixedProxyResolver was returned in a lot of cases where actually a library was called by a DELEGATECALL. I decided to only support EIP1167 proxies for this resolver. Any other FixedProxy would be a non-standardised proxy (or even a library). So in case a FixedProxyResolver is detected, we check explicitly for the opcodes standardised in EIP1167.
- I saw a few cases where multiple resolvers were returned, and also some cases where a resolver resolves to 0x0. The latter was for example the case for a factory contract which deploys EIP1967 contracts. Therefore, I iterate over the returned resolvers and return the first implementation address which is not 0x0.
This feature is only added to the /check-all-by-addresses endpoint and not the check-by-addresses endpoint, because in the latter the chainIds array does not contain objects.

Remaining issue (non-blocking)

There is still a problem left from shazow/whatsabi#142:
A contract that embeds a proxy (as for example a factory contract) could falsely be detected as a proxy.

A first proposal to solve this is to not consider contracts with CREATE/CREATE2 opcodes as proxies. However, this will not catch all contracts that only embed proxy bytecodes, and also exclude factory contracts that could be at the same time a proxy.

Edit: We decided that this problem is not a blocker for this PR. IMO, it is good to get this out and test it. The proxy data won't be persisted in our database with this change, so before we persist it we might be able to improve the proxy detection.

kuzdogan · 2024-10-31T07:38:22Z

Can you rebase this @manuelwedler I am seeing changes from #1689

kuzdogan

I'll further review after rebase

services/server/src/server/controllers/repository/repository.handlers.ts

kuzdogan · 2024-10-31T07:37:21Z

services/server/src/server/controllers/repository/repository.handlers.ts

+            chainId,
+            status: found[0].runtimeMatch,
+            ...proxyStatus,
+          });
        }
      } catch (error) {
        // ignore


Not introduced by this PR but not a good idea to ignore silently. We should log here

manuelwedler · 2024-10-31T09:10:28Z

Done. I think this was due to rewriting history. IMO, we should never do this again.

kuzdogan · 2024-10-31T09:39:33Z

Done. I think this was due to rewriting history. IMO, we should never do this again.

Right, but not never, IMO it was necessary in this case, unfortunately.

kuzdogan

Some comments and asking for explanations on the proxy resolution. In general looks good, and not a too big PR 👍

services/server/src/server/services/utils/proxy-contract-util.ts

services/server/src/server/controllers/repository/repository.handlers.ts

manuelwedler · 2024-11-25T05:47:54Z

Added more comments. Hope they are comprehensive enough

manuelwedler · 2024-11-25T06:35:38Z

I also updated whatsabi to the latest version, and added a check for factory contracts which is enabled by the latest version.

kuzdogan

Love it! The comments in the proxy-utils.ts make it a lot clearer now.

Just, proxy resolution is added to checkAllByChainAndAddress but not checkByChainAndAddress. I'm ok with not adding it to the latter endpoint but just wanted to bring it up.

kuzdogan · 2024-11-26T14:44:54Z

services/server/src/server/services/utils/proxy-contract-util.ts

+  const fixedProxy = proxyResolvers.find(
+    (proxy) => proxy instanceof whatsabi.proxies.FixedProxyResolver,
+  );
+  // Only return EIP1167Proxies because whatsabi can falsely detect non-proxy contracts as FixedProxies (e.g. libraries)
+  if (fixedProxy && isEIP1167Proxy(bytecode, fixedProxy.resolvedAddress)) {
+    return {
+      isProxy: true,
+      proxyType: "EIP1167Proxy",
+      implementations: [fixedProxy.resolvedAddress],
+    };
+  }


What's the difference between a FixedProxy and EIP1167Proxy?

The whatsabi returns a FixedProxy for anything called with a DELEGATECALL. I think the idea is to also catch non-standardized "minimal" proxies, but the problem with this approach is that it can also include libraries as seen in this issue: shazow/whatsabi#142

Therefore, I decided to only support the standard EIP 1167.

manuelwedler · 2024-11-27T08:01:30Z

Just, proxy resolution is added to checkAllByChainAndAddress but not checkByChainAndAddress. I'm ok with not adding it to the latter endpoint but just wanted to bring it up.

Unfortunately, the responds of the checkByChainAndAddress endpoint is structered differently and does not allow to add the proxy detection in a backwards-compatible way. I mentioned this in the PR description.

manuelwedler changed the title ~~API-level proxy detection and implementation resolution~~ Add API-level proxy detection and implementation resolution Oct 22, 2024

manuelwedler marked this pull request as ready for review October 24, 2024 12:01

kuzdogan self-assigned this Oct 28, 2024

kuzdogan force-pushed the staging branch from dbd18ca to 3db4b6d Compare October 29, 2024 09:13

kuzdogan reviewed Oct 31, 2024

View reviewed changes

manuelwedler force-pushed the proxy-resolution branch from ef2313b to fc49f87 Compare October 31, 2024 09:06

kuzdogan requested changes Nov 18, 2024

View reviewed changes

manuelwedler force-pushed the proxy-resolution branch 2 times, most recently from 6f40b57 to 234ecc5 Compare November 25, 2024 06:34

manuelwedler requested a review from kuzdogan November 25, 2024 06:57

kuzdogan approved these changes Nov 26, 2024

View reviewed changes

manuelwedler added 11 commits November 27, 2024 09:12

Add proxy detection and resolution to /check-all-by-addresses endpoint

c6a11af

Revise implementation

9e335c9

Add optional API parameter

1c02769

Add support for DiamondProxies

d004b72

Fix import

b8e725e

Add unit tests for proxy-contract-util

d0f76c6

Add integration tests for proxy resolution

01d4e48

Update whatsabi to v0.15.4

d750362

Log errors during checkAllByChainAndAddresses

92fc61e

Add comments

68efa3b

Update whatsabi to 0.17.0 and add check for ignoring factory contracts

9737163

manuelwedler force-pushed the proxy-resolution branch from 234ecc5 to 9737163 Compare November 27, 2024 08:12

manuelwedler merged commit ba3a8c8 into staging Nov 27, 2024
4 of 6 checks passed

manuelwedler deleted the proxy-resolution branch November 27, 2024 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API-level proxy detection and implementation resolution #1711

Add API-level proxy detection and implementation resolution #1711

manuelwedler commented Oct 22, 2024 •

edited

Loading

kuzdogan commented Oct 31, 2024

kuzdogan left a comment

kuzdogan Oct 31, 2024

manuelwedler commented Oct 31, 2024

kuzdogan commented Oct 31, 2024

kuzdogan left a comment

manuelwedler commented Nov 25, 2024

manuelwedler commented Nov 25, 2024

kuzdogan left a comment

kuzdogan Nov 26, 2024

manuelwedler Nov 27, 2024

manuelwedler commented Nov 27, 2024

Add API-level proxy detection and implementation resolution #1711

Add API-level proxy detection and implementation resolution #1711

Conversation

manuelwedler commented Oct 22, 2024 • edited Loading

Remaining issue (non-blocking)

kuzdogan commented Oct 31, 2024

kuzdogan left a comment

Choose a reason for hiding this comment

kuzdogan Oct 31, 2024

Choose a reason for hiding this comment

manuelwedler commented Oct 31, 2024

kuzdogan commented Oct 31, 2024

kuzdogan left a comment

Choose a reason for hiding this comment

manuelwedler commented Nov 25, 2024

manuelwedler commented Nov 25, 2024

kuzdogan left a comment

Choose a reason for hiding this comment

kuzdogan Nov 26, 2024

Choose a reason for hiding this comment

manuelwedler Nov 27, 2024

Choose a reason for hiding this comment

manuelwedler commented Nov 27, 2024

manuelwedler commented Oct 22, 2024 •

edited

Loading