ERC-7512: Proxy Support

[alex-ppg]

I have introduced support for associating an audit with a "contract" regardless of whether it is represented by a proxy implementation or is a standalone instance. Here's a brief changelog:

• Added support of a contract implementation that should be future-proof and support arbitrarily nested proxy chains
• Added security considerations in relation to Diamond pattern proxies as well as auditor key management with a potential future improvement

[rmeissner]

The challenge here is that to properly compare this it would be necessary to load the implementation from the proxy. Not sure if this is even always possible (i.e. the minimal proxy implementation does not allow this).

Also diamond proxies allow "partial" reuse of the implementation contract (per function based), which has a security impact too.

Therefore my motivation was to handle proxies outside of this ERC in a first version. I.e. there could be registries that are proxy aware. So they use the audits and then check for specific proxy implementations if they use that contract mentioned in the audit as a implementation.

function build1167ProxyCodehash(address proxy) internal returns (bytes32);

function checkAuditForProxy(address proxy, AuditSummary audit) public {
  required(proxy.codehash == build1167ProxyCodehash(audit.address), "Unsupported proxy");
  
  // verify AuditSummary
}

[alex-ppg]

Thanks for the feedback @rmeissner! I have included a disclaimer in relation to Diamond standards in the MD later on. In regard to the minimal proxy, it does actually allow it albeit using byte indices and loading the code of the deployed address in memory. Given that it is only a few bytes, it is trivial to implement a function that extracts the address.

As we also specify ERC support when submitting an audit, we can simply submit the proxy along with the proxy standard it supports and allow the ecosystem to build proper methods for extracting the proxy implementation address to validate audits.

We do not need to define the actual way the ecosystem extracts the necessary data entries to construct the bytes32 implementation, however, I think we should specify a future-proof way to handle how an audit is associated with a contract instance which can take many forms.

[rmeissner]

Wouldn't it make more sense then to add more explicit proxy information to the audit summary?

I.e. a boolean if it is a proxy, if it is upgradable and a name of the proxy (to indicate how to load the implementation address onchain).

My challenge here is that there are quite some proxy contracts (with and without standards) and I am not sure how to cover all of them, in a concise way.

[alex-ppg]

The standard itself will cover them regardless of whether they comply with known standards or not. At its core, it is a hash chain of address, runtimeCode consecutively. By definition, all proxies in any shape or form need to comply with this paradigm due to the usage of delegatecall (which is the only instruction that permits proxies in the first place).

How the ecosystem ends up actually extracting this information on-chain should not be a concern of the ERC. In regard to off-chain extraction of the implementation, it is much more trivial and could brute-force known standards or even fetch past transactions and inspect their call chain to see what addresses were delegatecall-ed to.

[rmeissner]

Ok, I think I am aligned with you now. Last minor nitpick is the name implementation (got me confused as the address usage is something different than the bytes32). It is like a checksum so using that as a name might be easier to understand.

[alex-ppg]

Good to hear, I have no objections to changing the name of the variable. I agree implementation is not the best description, checksum is more accurate and I am aligned with it. I will also propose digest which I think I've seen used in ERCs more often. Either is definitely better than implementation though as it may indeed mislead people at first glance.

[rmeissner]

The initial version of this ERC actually used runtimeCode instead of addresses (see https://hackmd.io/@rimeissner/erc-onchain-audit-representation). Here the comment was that due to the contract state the runtimeCode cannot be considered sufficient, and that some teams do checks on the deployment setup for a specific address.

It seems different auditors have different processes here and the question is how to include this in an initial version.

Potentially either having both, or allowing to choose will be necessary.

Personally I would opt for a combination of chainid, address, runtimeCode (and optionally even deploymentCode, but deployment code might be meaningless if a unknown factory was used)

[alex-ppg]

As this standard is meant to represent a ubiquitous case, I believe we should stick with solely the deployed code. Not all audit companies validate the actual arguments of the deployed contracts and the standard specifying that they are required would exclude them which can be considered unfair.

The current proposed case associates the combination you specified (chainId, address, runtimeCode) by having the chainId as a dedicated entry and the combination of an address and runtimeCode as part of the hash payload. A hash of the runtimeCode (which EXTCODEHASH yields) is equivalent to it and much cheaper than loading the full code in Solidity implementations.

[rmeissner]

ohh yeah I meant the runtimeCodeHash ... typo from my side 🙈

Just to clarify, so you would argue not to add deploymentCode (or anything similar). The combination of chainId, address, runtimeCodeHash would work.

Should address be optional?

[alex-ppg]

Yes, the deploymentCode should be avoided as it cannot be extracted on-chain and also requires additional effort from audit companies that may not be willing to provide it. The audit summary or another data point of the audit itself could have a bool flag that indicates the actual arguments of the deployment were "audited" or not.

RE: address, as this is a hash-chain, rendering it optional would require all integrators to calculate two hashes instead of one whenever evaluating an unknown contract (as an auditor may have submitted it with the address associated and without).

Additionally, we will face issues where the same bytecode has been audited multiple times by different auditors if we solely rely on the runetimeCodeHash.

Based on the above arguments, I think the address should be mandatory.