crytic/not-so-smart-contracts

Examples of Solidity security issues

No-so-smart-contracts is now in building-secure-contracts (see https://secure-contracts.com/)

(Not So) Smart Contracts

This repository contains examples of common Ethereum smart contract vulnerabilities, including code from real smart contracts. Use Not So Smart Contracts to learn about EVM and Solidity vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools.

Features

Each Not So Smart Contract includes a standard set of information:

• Description of the unique vulnerability type
• Attack scenarios to exploit the vulnerability
• Recommendations to eliminate or mitigate the vulnerability
• Real-world contracts that exhibit the flaw
• References to third-party resources with more information

Bonus! We have also included a repository and analysis of several honeypots.

Vulnerabilities

Not So Smart Contract	Description
Bad randomness	Contract attempts to get on-chain randomness, which can be manipulated by users
Denial of Service	Attacker stalls contract execution by failing in strategic way
Forced Ether Reception	Contracts can be forced to receive Ether
Incorrect Interface	Implementation uses different function signatures than interface
Integer Overflow	Arithmetic in Solidity (or EVM) is not safe by default
Race Condition	Transactions can be frontrun on the blockchain
Reentrancy	Calling external contracts gives them control over execution
Unchecked External Call	Some Solidity operations silently fail
Unprotected Function	Failure to use function modifier allows attacker to manipulate contract
Variable Shadowing	Local variable name is identical to one in outer scope
Wrong Constructor Name	Anyone can become owner of contract due to missing constructor

Credits

These examples are developed and maintained by Trail of Bits. Contributions are encouraged and are covered under our bounty program.

If you have questions, problems, or just want to learn more, then join the #ethereum channel on the Empire Hacking Slack or contact us directly.

not-so-smart-contracts's Issues

CLAAssistant do not work

I keep seing CLAAssistant in my PR despite fact that I submited work

Add examples to Bad Randomness

DoS writeup

Hi guys,

In the Denial-of-Service section it says "contracts that bulk perform transactions or updates using a for loop can be DoS'd if a call to another contract or send fails during the loop.".

AFAIK send() returns false if the call fails which means the loop would keep running. On the other hand, transfer() would propagate exceptions and cause a revert.

In the example:

contract CrowdFundSafe {
  address[] private refundAddresses;
  mapping(address => uint) public refundAmount;
  uint256 nextIdx;
  
  function refundSafe() public {
    uint256 i = nextIdx;
    while(i < refundAddresses.length && msg.gas > 200000) {
      refundAddresses[i].send(refundAmount[i]);
      i++;
    }
    nextIdx = i;
  }
}

IMO:

• refundAddresses[I].send failing would not cause the contract to revert (even using this pattern of has other issues, such as failing to notice when a user wasn't refunded successfully);

• If transfer() was used instead of send, a malicious contract could revert the call by throwing an exception in its fallback function;

• If call().value() was used to refund, a malicious contract could use up all gas;

• The other DOS vector would be to cause the list of funders to be to long, perhaps by sending small amounts of ETH from thousands of different addresses.

Note that I haven't actually tested any of this :)

New Reentrancy Vuln

Determine whether this is a different example than the one we already have and, if so, add it:

https://github.com/ConsenSys/smart-contract-best-practices#reentrancy

Add bad use of tx.origin

tx.origin should not be called for authorization.

See slither testcase to have examples of incorrect and legitimate usages:
https://github.com/trailofbits/slither/blob/2aea762600ebdb46798f53c7dd65ac375783c5dc/tests/tx_origin.sol

Security Scan of your project

We have scanned your project on https://scanmycode.io for Web2 & Web3 and Infractructure Cloudnative issues. Including: PHP, Java, Scala, Python, Ruby, Javascript, Typescript, GO, Solidity, DeFi Security, Infrastructure as a Code Best Practice and Security (Docker, Kubernetes (k8s), Terraform AWS, GCP, Azure), Secret Scanning, Dependency Confusion, Trojan Source, Open Source and Proprietary Checks. Feel free to go to https://www.scanmycode.io/defiscan/crytic/not-so-smart-contracts/ to check your report. If you have any questions or need support on the report please contact us directly. Thank you!

Transaction Ordering Vuln

Add an example like this: https://github.com/ConsenSys/smart-contract-best-practices#transaction-ordering-dependence

Add Time Dependence Vuln

see TheRun https://etherscan.io/address/0xcac337492149bdb66b088bf5914bedfbf78ccc18)

Update the Readme

DOS with Unexpected Revert Vuln

https://github.com/ConsenSys/smart-contract-best-practices#dos-with-unexpected-revert

Strange description in "missing constructor" section

From Missing Constructor: "If the constructor of a smart contract is not present (or not spelled the same way as the contract name), it is callable by anyone."

IMO this doesn't make 100% sense. If the constructor is not present then it just means that state vars aren't initialized, not that is callable by anyone (something that's not present can't be called).

Perhaps this describes the issue more accurately:

Wrongly named constructor: A function intended to be a constructor is named wrongly, which causes it to end up in the runtime bytecode instead of being a constructor.

Make a table for the honeypots on the readme

Integer overflow vulnerability not applicable in Solidity 0.8.0 and up

Prior to Solidity 0.8.0, arithmetic operations would always wrap in case of under- or overflow leading to widespread use of libraries that introduce additional checks.

Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, thus making the use of these libraries unnecessary.

source

Add Reference to “Send with Throw Griefing”

http://vessenes.com/ethereum-griefing-wallets-send-w-throw-considered-harmful/

Add an External Call Not Tested Vuln

see https://www.kingoftheether.com/postmortem.html

Add Out of Gas Vuln

here is a real example: https://www.reddit.com/r/ethereum/comments/4ghzhv/governmentals_1100_eth_jackpot_payout_is_stuck/

KingOfTheEtherThrone solc error

$ solc benchmarks/nssc/unchecked_external_call/KotET_source_code/KingOfTheEtherThrone.sol 
benchmarks/nssc/unchecked_external_call/KotET_source_code/KingOfTheEtherThrone.sol:36:62: Error: Expected ';' but got '}'
    modifier onlywizard { if (msg.sender == wizardAddress) _ }

solidity compile warnings which are errors

I am using solc version Version: 0.4.24+commit.e67f0147.Linux.g++

$ solc missing_constructor/Missing.sol
Missing.sol:23:23: Warning: Using contract member "balance" inherited from the address type is deprecated. Convert the contract to "address" type to access the member, for example use "address(contract).balance" instead.
       owner.transfer(this.balance);
                      ^----------^

and

$ solc reentrancy/DAO_source_code/DAO.sol 
reentrancy/DAO_source_code/DAO.sol:91:52: Error: Expected ';' but got '}'
    modifier noEther() {if (msg.value > 0) throw; _}

and two others. Look for "Unconfigured" in the chart in https://ethereumanalysisbenchmarks.github.io/nssc/index.html

Rubixi solc error

solc benchmarks/nssc/missing_constructor/Rubixi_source_code/Rubixi.sol 
benchmarks/nssc/missing_constructor/Rubixi_source_code/Rubixi.sol:21:9: Error: Expected ';' but got '}'
        }
        ^

Add a Race Condition Vuln