RFC - ERC-165s lack of impact

GalloDaSballo commented 1 year ago

The following is a discussion on how ERC-165 can be weaponized to create findings that are awarded, with little to no value and with no meaningful impact, under reasonable circumstances.

While ERC-165 is used as an example, the discussion could be extended to other EIPs for which the impact is theoretical in the majority of circumnstances and informational in the rest.

Rationale on the document

ERC-165 is an example of a EIP that by definition has no impact under rational pre-requisites

The ERC specifies a way to signal and determine if a certain interface is implemented

The wording of the ERC, which was published after EIP-1 which defined the wording to be used, is not clear and doesn't use the same keywords that other ERCs use.

More specifically the ERC doesn't even use the keywords MUST and SHOULD.

The only scenarios that have been offered over time are all strawman scenarios of an imaginary implementer that uses ERC-165 without actually reading the implementation of a contract. Or some appeal to a EIP compliance with little to no impact.

Recommendation

For EIPs that have opinions, the Judge should make their own judgment

If an EIP requires too many pedantic hypotheticals, with little to no impact, under any reasonable circumstances, the implementation of the EIP should not be considered a Medium Severity, but only a QA finding.

Plea for rationality and demonstration by absurd

By applying the myopic, zero-sum game approach that would justify a Medium Severity for lack of an interface implementation, I'll argue that ERC-165 compliance doesn't actually require implementing anything besides the interface and the functionality for the 2 instances that are a MUST.

The following is a discussion by absurd and hopefully we will be able to laugh about this discussion in the future

Impact

ERC-165 requires implementing the following interface

pragma solidity ^0.4.20;

interface ERC165 {
    /// @notice Query if a contract implements an interface
    /// @param interfaceID The interface identifier, as specified in ERC-165
    /// @dev Interface identification is specified in ERC-165. This function
    ///  uses less than 30,000 gas.
    /// @return `true` if the contract implements `interfaceID` and
    ///  `interfaceID` is not 0xffffffff, `false` otherwise
    function supportsInterface(bytes4 interfaceID) external view returns (bool);
}

While the author meant to say that ERC-165 MUST implement the function, they used the lowercase shall which is not to be confused with the rfc2119 defined MUST, meaning that technically speaking the ERC doesn't require any implementation

Conclusion 1

ERC-165 requires no code to have a compliant contract

Further Impact, no need for any implementation

By the same logic, we can quote the ERC

Therefore the implementing contract will have a supportsInterface function that returns:

true when interfaceID is 0x01ffc9a7 (EIP165 interface)
false when interfaceID is 0xffffffff
true for any other interfaceID this contract implements
false for any other interfaceID

Which is different from saying that the contract MUST return true for the interfaces that it implements.

Meaning that we can myopically conclude that a ERC-165 compliant contract is written in the following way

function supportsInterface(bytes4 interfaceID) external view returns (bool) {
  if(interfaceID == 0x01ffc9a7) { return true; }

  return false;
}

As such any contract that states they use ERC-165 would be compliant as long as it meets those 2 requirements

Any additional requirements, due to a lack of KEYWORDS would not be required.

Any lacking interface in this case would not be breaking the ERC, it would simply be signaling that the contract is not implementing the interface, which doesn't imply that the contract doesn't have implementation for said interface.

Obviously having the above contract is an exercise in futility, as it defeats the goal of the ERC.

However, as discussed above, this demonstrates how the logic that has been used in the past cuts both ways and can be used to dismantle the very ERC that was used to myopically generate findings.

Conclusion

Similar EIPs where the definitions are shaky, the impacts are non-existent, should not be considered for HM awards.

Appendix

Definitions of MUST and SHOULD: https://datatracker.ietf.org/doc/html/rfc2119

IllIllI000 commented 1 year ago

For EIPs that have opinions, the Judge should make their own judgment agree. As such any contract that states they use ERC-165 would be compliant as long as it meets those 2 requirements not sure which two requirements you're referring to, but openzeppelin will break with the version of the contract you're showing.

Also here's a direct link to the EIP

GalloDaSballo commented 1 year ago

Test are passing

Requirements are:

Return true for calling supportsInterface with param 0x01ffc9a7
Return false for calling supportsInterface with param 0xffffffff

IllIllI000 commented 1 year ago

ok thanks. I missed that your example contract was using 0x01ffc9a7 which is the EIP165 interface, and not some other arbitrary interface

JeffCX commented 1 year ago

Upvote, support, agree, please merge

DadeKuma commented 1 year ago

Agree for ERC-165 unless the implementation breaks this:

This function must return a bool and use at most 30,000 gas.

About this:

While ERC-165 is used as an example, the discussion could be extended to other EIPs for which the impact is theoretical in the majority of circumnstances and informational in the rest.

Agree only in cases where the EIP uses SHOULD, but MUST is considered a requirement, and failure to implement it should be considered valid. Interoperability issues between contracts are a security concern.

Why should someone implement an EIP and not follow the requirements? They were created to have common standards between external contracts.

GalloDaSballo commented 1 year ago

Agree for ERC-165 unless the implementation breaks this:

This function must return a bool and use at most 30,000 gas.

About this:

While ERC-165 is used as an example, the discussion could be extended to other EIPs for which the impact is theoretical in the majority of circumnstances and informational in the rest.

Agree only in cases where the EIP uses SHOULD, but MUST is considered a requirement, and failure to implement it should be considered valid. Interoperability issues between contracts are a security concern.

Why should someone implement an EIP and not follow the requirements? They were created to have common standards between external contracts.

If you desire to defend EIP-165, by the same logic you have to prove that there is a MUST in it. The word must is not the keyword MUST, as defined recursively by EIP-1 which uses RFC-2119.

gjaldon commented 1 year ago

https://github.com/code-423n4/2023-08-chainlink-findings/issues/522 - this ERC-165 issue looks like a valid MEDIUM to me because other contracts in the system expect the interface to be supported.

I think ERCs/EIPs should be respected and enforced because they are typically designed to help make the ecosystem more secure. Protocol integrations are always tricky and it is ERCs like ERC-165 that help make it less so.

Also, I think it is a clear CONFLICT OF INTEREST for @GalloDaSballo to be lobbying for invalidating ERC-165-related findings while judging is still ongoing for Chainlink. @GalloDaSballo has participated as a warden in that contest and, intentional or not, he stands to personally benefit from invalidating that ERC-165-related finding. It is unfair for other wardens for @GalloDaSballo to be able to exert such influence in his capacity as a Judge and as part of the C4 Supreme Court so that he benefits as a warden.

GalloDaSballo commented 1 year ago

1) In linking and commenting on an open issue, you are in fact in violation of the backstage rules, even more so than your accusation of my indirect influence of said outcome

In contrast to your comment, my discussion has no mention of specific findings and instead focuses on logical aspects

2)

I think ERCs/EIPs should be respected and enforced because they are typically designed to help make the ecosystem more secure. Protocol integrations are always tricky and it is ERCs like ERC-165 that help make it less so.

Statement that you conveniently make, which you would as rapidly disregard if this was taken at face value. If a protocol used such a EIP to determine the security of integration while accepting untrusted input, you'd send that as a finding to no-ones surprise, meaning your assertion is not just an opinion, but a poorly thought out one

3) Due to the mention of the conflict of interest I will forego all my winnings from said contest, either by forfeiture in the formula or by donating them to a charity of my choice

4) By similar logic, you can intentionally or not stand to benefit by mentioning the linked above finding as either you (under pseudonym), your friends or colleagues stand to benefit by the decision

5) As for the timing of the submission, I was asked to publish a issue wrt ERC-165 and similar by @dmvt and @trust1995 due to our work as the Supreme Court, and the discussion being a precursor to more decisions

IllIllI000 commented 1 year ago

I was confused about this whole org issue because it didn't link to any specific previous finding that would fall into its category. It did say, however, "lack of impact", so I assumed it would be restricted to such cases. If there is an impact as has been shown, then it should not be arbitrarily downgraded. @GalloDaSballo can you link to a separate public finding that you think it would apply to, and another that it would not, just so we're all on the same page?

IllIllI000 commented 1 year ago

5. As for the timing of the submission, I was asked to publish a issue wrt ERC-165 and similar by @dmvt and @trust1995 due to our work as the Supreme Court, and the discussion being a precursor to more decisions

that sounds like the court already has an outcome/agenda that they're trying to push, rather than taking up issues that the community has actually struggled with and have strong documented opinions on from both sides (process says Look at instances where the relevant situation has been judged (i.e. what's the judging track record? Is there emerging consensus or prevailing sentiment?)). There is value in not addressing some issues until they're fully fleshed out

CjHare commented 1 year ago

Sounds sensible enough! Unless an issue actually has sufficient impact (i.e. demonstrated risk to the protocol with example) ...why should it be awarded a H/M?

Similar EIPs where the definitions are shaky, the impacts are non-existent, should not be considered for HM awards. Presumably this would not exclude valid, impactful issues from being H/M simply if they are merely centered around those EIPs?

e.g. EIP-165 being used by a protocol internally, where an identified incorrect implementation would brick the protocol (one could assume such a thing should be picked up during integration testing, but there is a saying about assumptions)