solidity
solidity copied to clipboard
ethereum
Explained Checks-Effects-Interactions and added info on Checks-Effects-Events-Interactions
I can't speak for the correctness of the content, but to get your CI tests fixed you need to rebase on our latest develop. Further, please amend the commits into one (I see a merge commit)
@leonardoalt it's a dangerous precedent though to say to your users "you can use this, but you have to be smarter than your potential attackers at reasoning your way through a complex set of logic that proves than any state they can manipulate in the middle of a transaction will ultimately have no negative effect on any user". Far better to make a blanket statement that calling any contract before state is finalized is extremely likely to open you to security vulnerabilities, which it is, and that these security vulnerabilities have lost hundreds of millions of dollars for victims, which they have.
@leonardoalt it's a dangerous precedent though to say to your users "you can use this, but you have to be smarter than your potential attackers at reasoning your way through a complex set of logic that proves than any state they can manipulate in the middle of a transaction will ultimately have no negative effect on any user". Far better to make a blanket statement that calling any contract before state is finalized is extremely likely to open you to security vulnerabilities, which it is, and that these security vulnerabilities have lost hundreds of millions of dollars for victims, which they have.
Sure, I haven't advocated for that at all. What I disagree with is the imprecise wording in the particular line I commented. I'm all for recommending CEIs, but it is simply incorrect to say that this is the only way.
OK, I understand your point better now. Thanks for the feedback.
My main reasons for creating this PR is that the current wording is a bit too weak -- users need to know what a huge source of potential vulnerabilities it is to call external contracts before finalizing state -- and also to point out that events should also only be emitted after finalizing state, because otherwise it is possible to trick 3rd parties into believing false things about the final state of the contract after the transaction. (This latter point is raised almost nowhere.)
I'm happy for my suggestion to be completely rewritten, taking these points into account. Unfortunately I don't have bandwidth for this, I was just trying to be helpful by submitting a ready-made change.
Sounds good, thanks for it. If you don't mind then someone from the team will rewrite some of the wording and we'll bring it up in the call whether we want to have a section for CEIs.
Please go ahead, thanks. You can close this PR whenever somebody writes something better!
I tried pushing my changes directly to the given repo/branch, which is usually possible when external contributors make PRs to this repo, but this was not possible. So I made another branch/PR: https://github.com/ethereum/solidity/pull/13457 (keeping author's credit) and am closing this one.