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

Import not-so-smart-contracts #146

Open
wants to merge 74 commits into
base: master
Choose a base branch
from
Open

Import not-so-smart-contracts #146

wants to merge 74 commits into from

Conversation

bohendo
Copy link
Contributor

@bohendo bohendo commented Oct 3, 2022

This PR imports all content (including the git history) from crytic/not-so-smart-contracts and places it in the not-so-smart-contracts/solidity directory.

Important note: none of the PRs to crytic/not-so-smart-contracts have been imported but, if/when this PR gets merged, I'll help import whichever PRs are worth importing (3 of 6 PRs are >2 years old so probably not worth it but tbd).

montyly and others added 30 commits August 28, 2017 18:26
- DAO
- Integer overflow
 - missing constructor
 - unprotected function
 - wrong interface
Add DAO and Paritity Wallet source code
We don't use the word 'vulerability' in the other titles; it is assumed
Initial work on #4
Start on #5
However I am told that in the 60's there was a Fortan compiler used developed
Brooklyn NY, that accepted INTERGER any place INTEGER was expected.

This was touted as an early innovation in user-friendliness.
albeit with numerous warnings.
Add a solidity pragma.
@bohendo bohendo requested review from montyly and 0xalpharush October 3, 2022 21:15
@CLAassistant
Copy link

CLAassistant commented Oct 3, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
7 out of 8 committers have signed the CLA.

✅ montyly
✅ ESultanik
✅ ggrieco-tob
✅ dguido
✅ tayvano
✅ mgcolburn
✅ blperez01
❌ bohendo


bohendo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bohendo
Copy link
Contributor Author

bohendo commented Oct 3, 2022

I'm unfamiliar with CLA. If I need to clobber the contributions by offlinemark, perks, rocky, and lojikil and rewrite those commits to be authored by myself, lmk & I can figure that out.

@montyly
Copy link
Member

montyly commented Oct 4, 2022

  • We can remove not-so-smart-contracts/solidity/LICENSE
  • I think some of these issues need to be refreshed. For example
    • The integer overflow does not contain any mention of solidity 0.8
    • Wrong Constructor Name is not an issue anymore
  • This PR contains a couple of additional (and more recent) issues: Add 10 vulnerabilities not-so-smart-contracts#38

Overall I am not sure we should import the git history and the whole repo. Not so smart contract hasnt been updated in years, and as a result it's really not up to date. I think we should start by picking the ones that are still true, PR38, and probably a couple of issues highlighted in slither private detectors

@bohendo
Copy link
Contributor Author

bohendo commented Oct 4, 2022

Overall I am not sure we should import the git history and the whole repo. Not so smart contract hasnt been updated in years, and as a result it's really not up to date.

I like importing git history like this because it preserves contribution history & git blame output. I agree that this content needs an editing pass, but IMO we should edit & prune on top of past contributions to preserve a record of the great work others have contributed so far. TBD tho, I'm looking into CLA stuff now & if this causes too much friction then I'll close this PR in favor of a more selective copy/paste.

Wrong Constructor Name is not an issue anymore

This isn't the only one that's not an issue if a modern solidity compiler is used. In these cases, would you recommend adding a mention of which solidity versions are vulnerable or removing these items entirely?

a couple of issues highlighted in slither private detectors

Agreed that adding more content to this section is worthwhile, but IMO that's out of scope for this particular PR. Let's focus first on what should NOT be imported before we start considering what additional content would help make this section great.

@bohendo
Copy link
Contributor Author

bohendo commented Oct 4, 2022

Update: I've given this whole thing a thorough editing pass. All solidity examples have been linted & reformatted & upgraded to the latest solidity version in which the issue is still applicable (mostly 0.8.x except for integer overflows & honeypots)

For very old issues, the honeypots ended up serving as a great gallery of historical gotchas. For example, the VarLoop honeypot takes advantage of old fashioned constructor name mismatches so I've removed the dedicated not-so-smart-example for that issue.

This all fits together nicely now IMO: most of these examples are targeting people developing new contracts so issues with ancient versions of solc aren't so important. But honeypots are adversarial & will use whichever version of solc suits their purposes so using those as a gallery of historical solc issues makes sense.

I'm very happy with the honeypot page now: I used some in-line html to hide the description of each trap description giving the reader a chance to browse the intro + contract source before revealing what the trap is.

@ggrieco-tob
Copy link
Member

What is the status of this? Should we have another review?

@montyly
Copy link
Member

montyly commented Feb 17, 2023

This is on hold. Nsc for solidity is outdated and need a refresher

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

Successfully merging this pull request may close these issues.

8 participants