This repository has been archived by the owner on Feb 24, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 342
Add 10 vulnerabilities #38
Open
montyly
wants to merge
1
commit into
master
Choose a base branch
from
10-new-vulnerabilities
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
## State variable shadowing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no link to this document in the main README.md. |
||
|
||
### 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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this makes sense but should there be a generalized rule that all interfaces should be correctly configured to return the correct value? This same type of bug could occur in cases where a developer uses a custom or non ERC-20/721 interface?