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

Call for Input: Allow Appending to Security Considerations in Final Proposals #349

Closed
9 tasks done
SamWilsn opened this issue Jul 17, 2024 · 15 comments
Closed
9 tasks done

Comments

@SamWilsn
Copy link
Collaborator

SamWilsn commented Jul 17, 2024

Call for Input

Decision

Do we allow modifications to Final proposals when all of the following are met:

  • The modification is only appending new content to the Security Considerations section;
  • The modification describes a consideration that has caused financial loss.
If Affirmed

EIP-1 is modified to allow appending to Security Considerations.

If Rejected

No change.

Method

Rough Consensus

Deadline

August 16, 2024

Background

@Dexaran has a detailed comment here: #340 (comment)

Checklist

I, the opener of this Call for Input, have completed the following:

  • Filled in a descriptive title.
  • Filled in the "Decision" field.
  • Filled in the "If Affirmed" field.
  • Filled in the "If Rejected" field.
  • Filled in the "Method" field.
  • Filled in the "Deadline" field to be thirty days from creation.
  • Added any relevant background information (or removed the section.)
  • Published a notice in writing to the usual channels frequented by EIP Editors (likely Discord.)
  • Commented on this Call for Input, clearly stating my opinion (or abstention.)
@g11tech
Copy link

g11tech commented Jul 17, 2024

in favor

to keep the community abreast

@SamWilsn
Copy link
Collaborator Author

I am opposed.


As I have stated several times, I do not want EIP Editors to be in a position where we have to decide what is or isn't a Security Consideration.

"Financial loss" is not sufficiently objective nor specific. I'm sure that 90% of opcodes have been involved in a hack in one way or another, and I don't believe we should be updating those opcode's EIPs with notes on each hack.

IMHO, the correct solution to this problem would be a low-barrier wiki where anyone can enumerate whatever considerations they want about an EIP. If the wiki page contains, for example, a critical admonition then the renderer can insert a note on the EIP in question:

Screenshot of mockup of notice on EIP

@Dexaran
Copy link

Dexaran commented Jul 17, 2024

Development must be driven by real problems. The current "final = no change" rule resulted in a situation where problems known for years keep causing financial damage to users.

In my original proposal I was recommending to set a minimal threshold of financial losses that must be transparently verifiable in order to add an issue to the Security Considerations of the corresponding EIP. There are some hypothetical issues like "it is possible to deliver Ether to a contract bypassing the fallback function or receive() function invocation via the SELFDESTRUCT" but this issue is already well-documented and it is not possible to lose any funds accidentally because of this issue.

The fact that someone deposited 1 wei to the USDT contract does not indicate that it's a huge problem that must be considered a security issue. (Though there is nothing wrong with adding it to the SELFDESTRUCT-related EIP texts if they exist. It's still better to have all types of issues documented including the not-so-important ones rather than not to have critical issues documented).

EIP editors are not security experts

@SamWilsn repeatedly stated that placing a burden of determining if something is a security problem or not on EIP editors is unreasonable. In my proposal EIP editors are only supposed to perform actions which do not require any security expertise.

It is not necessary to assign severity to the issues being added to the Security Considerations. We are just documenting everything which can cause a financial damage without stating if this is a "security vulnerability", "flaw" or something else. Blockchains are mostly about financial infrastructure and this step is supposed to prevent financial damage to the end users.

Additionally, EIP editors are not expected to evaluate reported issues themselves, but will instead perform actions that any average Internet user could perform (and therefore everyone can verify the EIP editors decisions): open links, read what's there, identify if the information is real or not.

For example here is a transaction where some user lost 33,713 USDT: https://etherscan.io/tx/0x28d62d10b514b1f4f1450b12121b2b199c2d68088f35906504f66bc9d00b6009

I assume that EIP editors can identify that etherscan.io is a valid blockchain explorer, the USDT is a real token and it's price is $1 = 1 USDT, the funds are deposited to a contract which doesn't have a function to extract them.

Or there is a similar example with $130,000 7 years ago: https://www.reddit.com/r/ethtrader/comments/7lplwk/ive_accidentally_sent_130000_worth_of_salt_tokens/

The issue is know, the issue is real, the issue is causing damage to real humans and there is no way to provide an official documentation for the implementors so that they would stop inheriting it. My proposal is supposed to solve this.

Opcodes could be involved in a hack

I assumed that EIP editors could identify a difference between "a bug in a particular implementation" and "a problem of a standard or a proposal".

Imagine a situation where a programmer used = instead of += operation in the code and as a result his software assigned balance instead of increasing it (and therefore the previous value was erased).

Is it a problem of an assignment operation =? - No.
Was it possible to write a program that wouldn't have this problem but would still use assignment operations? - Yes.

Is it a problem of an addition operation +=? - No.
Was it possible to write a program that wouldn't have this problem but would still use addition operations? - Yes.

This example is a typo in the code of a particular program. Not a general issue.

In case of hacks they don't exploit vulnerabilities in opcodes, they exploit vulnerabilities in the logic of software that uses this opcodes.

Example 1: ERC-1363 tokens can be deposited to counterfactual contracts. Is it a problem of ERC-1363? - No, this is not a problem with ERC-1363, this is a problem with someone's idea to think of addresses that may (or may not) become contracts under some circumstances as if they are already contracts before the bytecode is deployed.

Is it possible to solve this on the application level? - No. Native currency (ether), ERC-20, ERC-223, NFT ERC-721 and all other classes of assets would suffer from the same issue because this is not even an application-level issue.

Example 2: ERC-721 tokens can be transferred to addresses not owned by anyone. Is it a problem of ERC-721? - No, this is not a problem with ERC-721, this is a problem of how addresses are generated/derived in Ethereum. There is no way to identify if an address is owned by anyone or not on the application level.

Is it possible to solve this on the application level? - No. Native currency (ether), ERC-20, ERC-223, NFT ERC-721, ERC-1363 and all other classes of assets would suffer from the same issue because this is not an application-level issue.

Example 3: ERC-20 tokens can be transferred to addresses of contracts which are not designed to receive them. Is it a problem of ERC-20? - Yes, this one is a problem of this particular standard.

Is it possible to solve this on the application level? - Yes it is. It is possible to (1) restrict the logic of the ERC-20 transfer function and (2) it is possible to replace two different methods transfer and approve + transferFrom with just one transferring method that would invoke a transfer handler function in the recipient's address and revert() if it's not implemented.

You can't lose your native currency (ether), ERC-223 tokens or ERC-721 NFTs because of this issue. Those ARE NOT affected.

You can lose your ERC-20 tokens or ERC-1363 tokens because of this issue. Those ARE affected.

If @SamWilsn thinks its too difficult for EIP editors to determine what is the cause of the problem and what is not then we can restrict my proposal to application-level standards only. I think this is what ERCs currently represent. My proposal was originally designed for application-level standards so in case of core EIPs it may be necessary to design a different procedure.

In my opinion it's better to have extra issue records in EIPs rather than not to have critical issue records and keep implementors inheriting them for ages.

The correct solution to this problem would be a low-barrier wiki

We've already tried this. In my opinion this creates more problems with the process. What rules do we need to implement in the wiki? Who will maintain it? How are security issues added there? How do we prevent bloating it with false reports?

I recall we had such a discussion for several years and still didn't came to a conclusion. For example this is the last one https://ethereum-magicians.org/t/modification-of-eip-process-to-account-for-security-treatments/16265

My proposal is supposed to take care of the most important problems (direct loss of funds for the end users) without unnecessary delays.

Again, people keep losing money for 7 years because of well-known issues while discussions are being discussed. The transaction that I provided above happened just yesterday and someone lost years-worth-of-salary to an easily fixable software issue.

@abcoathup
Copy link

I'm against. Final is final.

I'd like security considerations to be added to the top of the Eth Magicians discussions thread. Ideally as a wiki post, otherwise by the ERC author. Responsibility for maintaining should be with the ERC authors.

If they must be included in the ERC, then it should be append only and have multiple ERC authors agreeing. The responsibility shouldn't be with the ERC editors.

@ghiocel30
Copy link

Dexaran,They don't care that people lose money.

@kaviyarasi1
Copy link

In favor

The loss of millions of ETH due to ERC20 flaws primarily stems from vulnerabilities in smart contracts built on the Ethereum blockchain.

@Dexaran
Copy link

Dexaran commented Jul 18, 2024

@abcoathup

Responsibility for maintaining should be with the ERC authors.

Conflict of interests.

  • "Do you agree that the proposal that you created contains a security issue that caused $160M loss?"
  • Author: "No."
  • "Ok, we treat it as if it's secure."

Responsibility for maintaining should be with the ERC authors. ... The responsibility shouldn't be with the ERC editors.

Definitely will not work.

I've created ERC-223 few years ago, what if I don't care about it anymore? Will it result in impossibility of communicating it's problems to the implementors without my approval?

Or what will happen to it when I'll die? Will it get stuck without a possibility of reporting issues because the author is no longer available?

This is not a valid process of handling issue reports, it will not work, it will result in more financial damage to the users.

@TBugelman
Copy link

TBugelman commented Jul 18, 2024

Objectively, when I want to write an application with logic on smart contracts, I will refer to the documentation of the Solidity language and Ethereum as the logic execution platform. I find code on official resources that is marked as standard and has been used for many years. It never crosses my mind that there could be problems with it. Why should I put myself and my clients at risk? We're working with money, we're not writing indie games and selling them on Steam for $5. Dexaran's words are logical. I'm all for being able to see markings and warnings about problems in ERC and EIP text.

@Souptacular
Copy link
Contributor

Opposed

I am opposed for 2 reasons.

  1. EIPs are technical standards and once one is final discussion about them is better left to other forums. That includes security issues, improvement ideas, benchmarks, specific implementations in various languages/codebases.

  2. It's messy and would disrupt the decorum of the EIP process. It is not great that people lose money, but some smart contract "bugs" that causes fund movement or fund loss have been described as "profitable trading strategies. It is not as easy as looking at Etherscan, seeing that an EIP was involved, and calculating the total amount lost on that day and why.

In your response to the idea of a low barrier Wiki you said:

We've already tried this. In my opinion this creates more problems with the process. What rules do we need to implement in the wiki? Who will maintain it? How are security issues added there? How do we prevent bloating it with false reports?

Those questions would also apply to EIPs, as would anyone arguing to add benchmarks or other implementations in the EIP after final status.

Solution

A perfect place to create a resource like this is under SEAL (Security Alliance) - https://x.com/_SEAL_Org?t=RjELfykgsmQgYnIrTX9nOA&s=09.
Research the current initiatives and feel free to rally volunteers to start an external resource for this. SEAL is a coordination layer for these type of things (see SEAL911 and Whitehat Safe Harbor Contract).

Lastly, just because people opposed this idea doesn't mean they don't care about loss of funds. The resources to prevent that need to be used in the correct direction to limit wasted time. For example, instead of including post-final security considerations in the EIP (which many people don't even scroll down to see). Make PRs to put warnings in major repos that create frequently used contract templates, like OpenZeppelin or Solady.

@Dexaran
Copy link

Dexaran commented Jul 18, 2024

@Souptacular thanks for the feedback. I would like to highlight that the idea that you shared doesn't work in practice however, so a different approach is required

instead of including post-final security considerations in the EIP (which many people don't even scroll down to see). Make PRs to put warnings in major repos that create frequently used contract templates, like OpenZeppelin or Solady.

Conflict of interests.

OpenZeppelin runs auditing services and brands themselves as "experts". After they've audited 500 ERC-20 smart-contracts they will not put a warning about it's problems on their implementation because it will damage their reputation.

I reported this to OpenZeppelin 3 times:

They told me "if something is described as a standard - we will not change it. Go propose changes on Ethereum, if they will change it then we will reimplement it".

And in 2023 they refused to add it to their documentation as a known issue (as I've said, because they audited a lot of contracts by that time and they don't want to invalidate the results of their audits).

So, here in EIPs you tell me "go convince the implementers", when I'm talking to implementers they tell me "go convince the EIP editors" and actually nobody is taking on the responsibility.

@TBugelman
Copy link

"It is not great that people lose money, but some smart contract "bugs" that causes fund movement or fund loss have been described as "profitable trading strategies."

Sorry, could you explain what a profitable trading strategy means when the funds were sent and frozen forever in the contract, without following any logic at all?

@TechGuru20
Copy link

Opposed

The proposal's source lacks credibility and has a history of questionable behavior. He betrayed his callisto network community and now wants to gain attention from Ethereum developers to continue his malicious activities.

Not everyone who helps you has your best interests at heart.

@poojaranjan poojaranjan mentioned this issue Jul 20, 2024
10 tasks
@poojaranjan poojaranjan mentioned this issue Aug 21, 2024
3 tasks
@lightclient
Copy link

also opposed

@poojaranjan poojaranjan mentioned this issue Aug 21, 2024
23 tasks
@SamWilsn
Copy link
Collaborator Author

SamWilsn commented Sep 25, 2024

@axic @gcolvin @xinbenlv consider this the last call for this discussion. We have one in favour and two opposed.

@poojaranjan poojaranjan mentioned this issue Oct 16, 2024
16 tasks
@SamWilsn
Copy link
Collaborator Author

Closing as two opposed, one in favour. Not really a great consensus. There will be no appending to security considerations.

@SamWilsn SamWilsn closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants