search

ethcatherders / EIPIP

EIP Improvement Process
81 stars 37 forks source link

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

Open SamWilsn opened 1 month ago

SamWilsn commented 1 month ago

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: https://github.com/ethcatherders/EIPIP/issues/340#issuecomment-2207486824

Checklist

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

g11tech commented 1 month ago

in favor

to keep the community abreast

SamWilsn commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

kaviyarasi1 commented 1 month ago

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 commented 1 month ago

@abcoathup

Responsibility for maintaining should be with the ERC authors.

Conflict of interests.

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

"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 commented 1 month ago

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.

lightclient commented 2 weeks ago

also opposed