diff --git a/README.md b/README.md index 22aabdf..28326a9 100644 --- a/README.md +++ b/README.md @@ -19,17 +19,27 @@ Bonus! We have also included a repository and analysis of several [honeypots](ho | Not So Smart Contract | Description | | --- | --- | | [Bad randomness](bad_randomness) | Contract attempts to get on-chain randomness, which can be manipulated by users | +| [Dangerous Strict Equalities](dangerous_strict_equalities) | Use of strict equalities that can be easily manipulated by an attacker. | | [Denial of Service](denial_of_service) | Attacker stalls contract execution by failing in strategic way | | [Forced Ether Reception](forced_ether_reception) | Contracts can be forced to receive Ether | +| [Incorrect ERC20 Interface](incorrect_erc20_interface) | Token not implementing the ERC20 interface correctly. | +| [Incorrect ERC721 Interface](incorrect_erc721_interface) | Token not implementing the ERC721 interface correctly. | | [Incorrect Interface](incorrect_interface) | Implementation uses different function signatures than interface | | [Integer Overflow](integer_overflow) | Arithmetic in Solidity (or EVM) is not safe by default | | [Race Condition](race_condition) | Transactions can be frontrun on the blockchain | | [Reentrancy](reentrancy) | Calling external contracts gives them control over execution | +| [rtlo](rtlo) | Usage of malicious unicode character. | +| [Suicidal](suicidal) | Contract that can be destructed by anyone. | +| [Tautology](tautology) | Usage of always boolean expressions that are always true. | | [Unchecked External Call](unchecked_external_call) | Some Solidity operations silently fail | +| [Uninitialized State Variables](uninitialized-state-variables) | State variables that are used before being initialized. | +| [Uninitialized Storage Variables](uninitialized-storage-variables) | Storage variables that are used before being initialized. | | [Unprotected Function](unprotected_function) | Failure to use function modifier allows attacker to manipulate contract | +| [Unused Return Value ](unused-return) | Return values from calls that is not used. | | [Variable Shadowing](variable%20shadowing/) | Local variable name is identical to one in outer scope | | [Wrong Constructor Name](wrong_constructor_name) | Anyone can become owner of contract due to missing constructor | + ## Credits These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/). Contributions are encouraged and are covered under our [bounty program](https://github.com/trailofbits/not-so-smart-contracts/wiki#bounties). diff --git a/dangerous_strict_equalities/README.md b/dangerous_strict_equalities/README.md new file mode 100644 index 0000000..1abf38a --- /dev/null +++ b/dangerous_strict_equalities/README.md @@ -0,0 +1,19 @@ +## Dangerous strict equalities + +### Description +Use of strict equalities that can be easily manipulated by an attacker. + +### Exploit Scenario: + +```solidity +contract Crowdsale{ + function fund_reached() public returns(bool){ + return this.balance == 100 ether; + } +``` +`Crowdsale` relies on `fund_reached` to know when to stop the sale of tokens. `Crowdsale` reaches 100 ether. Bob sends 0.1 ether. As a result, `fund_reached` is always false and the crowdsale never ends. + +### Mitigations +- Don't use strict equality to determine if an account has enough ethers or tokens. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + diff --git a/incorrect_erc20_interface/README.md b/incorrect_erc20_interface/README.md new file mode 100644 index 0000000..50874cd --- /dev/null +++ b/incorrect_erc20_interface/README.md @@ -0,0 +1,21 @@ +## Incorrect erc20 interface + +### Description +Incorrect return values for ERC20 functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. + +### Exploit Scenario: + +```solidity +contract Token{ + function transfer(address to, uint value) external; + //... +} +``` +`Token.transfer` does not return a boolean. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC20 interface implementation. Alice's contract is unable to interact with Bob's contract. + +### Recommendation +- Set the appropriate return values and value-types for the defined ERC20 functions. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue +- Use `slither-check-erc` to ensure ERC's conformance + + diff --git a/incorrect_erc721_interface/README.md b/incorrect_erc721_interface/README.md new file mode 100644 index 0000000..3d98f25 --- /dev/null +++ b/incorrect_erc721_interface/README.md @@ -0,0 +1,20 @@ +## Incorrect erc721 interface + +### Description +Incorrect return values for ERC721 functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. + +### Exploit Scenario: + +```solidity +contract Token{ + function ownerOf(uint256 _tokenId) external view returns (bool); + //... +} +``` +`Token.ownerOf` does not return an address as ERC721 expects. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC721 interface implementation. Alice's contract is unable to interact with Bob's contract. + +### Mitigations +- Set the appropriate return values and value-types for the defined ERC721 functions. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + + diff --git a/rtlo/README.md b/rtlo/README.md new file mode 100644 index 0000000..ada8f36 --- /dev/null +++ b/rtlo/README.md @@ -0,0 +1,39 @@ +## Right-To-Left-Override character + +### Description +An attacker can manipulate the logic of the contract by using a right-to-left-override character (U+202E) + +### Exploit Scenario: + +```solidity +contract Token +{ + + address payable o; // owner + mapping(address => uint) tokens; + + function withdraw() external returns(uint) + { + uint amount = tokens[msg.sender]; + address payable d = msg.sender; + tokens[msg.sender] = 0; + _withdraw(/*owner‮/*noitanitsed*/ d, o/*‭ + /*value */, amount); + } + + function _withdraw(address payable fee_receiver, address payable destination, uint value) internal + { + fee_receiver.transfer(1); + destination.transfer(value); + } +} +``` + +`Token` uses the right-to-left-override character when calling `_withdraw`. As a result, the fee is incorrectly sent to `msg.sender`, and the token balance is sent to the owner. + + + +### Mitigations +- Special control characters must not be allowed. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + diff --git a/state-variable-shadowing/README.md b/state-variable-shadowing/README.md new file mode 100644 index 0000000..a50dcf3 --- /dev/null +++ b/state-variable-shadowing/README.md @@ -0,0 +1,37 @@ +## State variable shadowing + +### Description +State variables can be shadowed in Solidity. + +### Exploit Scenario: + +```solidity +contract BaseContract{ + address owner; + + modifier isOwner(){ + require(owner == msg.sender); + _; + } + +} + +contract DerivedContract is BaseContract{ + address owner; + + constructor(){ + owner = msg.sender; + } + + function withdraw() isOwner() external{ + msg.sender.transfer(this.balance); + } +} +``` +`owner` of `BaseContract` is never assigned and the modifier `isOwner` does not work. + +### Mitigations +- Avoid state variable shadowing. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + + diff --git a/suicidal/README.md b/suicidal/README.md new file mode 100644 index 0000000..33f65a2 --- /dev/null +++ b/suicidal/README.md @@ -0,0 +1,21 @@ +## Suicidal contract + +### Description +Unprotected call to a function executing `selfdestruct`/`suicide`. + +### Exploit Scenario: + +```solidity +contract Suicidal{ + function kill() public{ + selfdestruct(msg.sender); + } +} +``` +Bob calls `kill` and destructs the contract. + +### Mitigations +- Protect access to all sensitive functions. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + + diff --git a/tautology/README.md b/tautology/README.md new file mode 100644 index 0000000..c355696 --- /dev/null +++ b/tautology/README.md @@ -0,0 +1,32 @@ +## Tautology or contradiction + +### Description +Expressions that are tautologies or contradictions. + +### Exploit Scenario: + +```solidity +contract A { + function f(uint x) public { + // ... + if (x >= 0) { // bad -- always true + // ... + } + // ... + } + + function g(uint8 y) public returns (bool) { + // ... + return (y < 512); // bad! + // ... + } +} +``` +`x` is an `uint256`, as a result `x >= 0` will be always true. +`y` is an `uint8`, as a result `y <512` will be always true. + + +### Mitigations +- Avoid tautology +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + diff --git a/uninitialized-state-variables/README.md b/uninitialized-state-variables/README.md new file mode 100644 index 0000000..84abf62 --- /dev/null +++ b/uninitialized-state-variables/README.md @@ -0,0 +1,25 @@ +## Uninitialized state variables + +### Description +Usage of uninitialized state variables. + +### Exploit Scenario: + +```solidity +contract Uninitialized{ + address destination; + + function transfer() payable public{ + destination.transfer(msg.value); + } +} +``` +Bob calls `transfer`. As a result, the ethers are sent to the address 0x0 and are lost. + + +### Mitigations +- Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + + + diff --git a/uninitialized-storage/README.md b/uninitialized-storage/README.md new file mode 100644 index 0000000..b2ef953 --- /dev/null +++ b/uninitialized-storage/README.md @@ -0,0 +1,30 @@ + +## Uninitialized storage variables + +### Description +An uinitialized storage variable will act as a reference to the first state variable, and can override a critical variable. + +### Exploit Scenario: + +```solidity +contract Uninitialized{ + address owner = msg.sender; + + struct St{ + uint a; + } + + function func() { + St st; + st.a = 0x0; + } +} +``` +Bob calls `func`. As a result, `owner` is override to 0. + + +### Mitigations +- Initialize all the storage variables. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + + diff --git a/unused_return/README.md b/unused_return/README.md new file mode 100644 index 0000000..f14cb1c --- /dev/null +++ b/unused_return/README.md @@ -0,0 +1,23 @@ +## Unused return + + +### Description +The return value of an external call is not checked. + +### Exploit Scenario: + +```solidity +contract MyConc{ + using SafeMath for uint; + function my_func(uint a, uint b) public{ + a.add(b); + } +} +``` +`MyConc` calls `add` of SafeMath, but does not store the result in `a`. As a result, the computation has no effect. + +### Mitigations +- Ensure that all the return values of the function calls are used. +- Use [Slither](https://github.com/crytic/slither/) or [crytic.io](https://crytic.io/) to detect the issue + +