-
Notifications
You must be signed in to change notification settings - Fork 980
Detector Documentation
List of public detectors
- Check:
name-reused - Severity:
High - Confidence:
High
If a codebase has two contracts the similar names, the compilation artifacts will not contain one of the contracts with the duplicate name.
Bob's truffle codebase has two contracts named ERC20.
When truffle compile runs, only one of the two contracts will generate artifacts in build/contracts.
As a result, the second contract cannot be analyzed.
Rename the contract.
- Check:
rtlo - Severity:
High - Confidence:
High
An attacker can manipulate the logic of the contract by using a right-to-left-override character (U+202E).
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.
Special control characters must not be allowed.
- Check:
shadowing-state - Severity:
High - Confidence:
High
Detection of state variables shadowed.
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.
Remove the state variable shadowing.
- Check:
suicidal - Severity:
High - Confidence:
High
Unprotected call to a function executing selfdestruct/suicide.
contract Suicidal{
function kill() public{
selfdestruct(msg.sender);
}
}Bob calls kill and destructs the contract.
Protect access to all sensitive functions.
- Check:
uninitialized-state - Severity:
High - Confidence:
High
Uninitialized state variables.
contract Uninitialized{
address destination;
function transfer() payable public{
destination.transfer(msg.value);
}
}Bob calls transfer. As a result, the Ether are sent to the address 0x0 and are lost.
Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero.
- Check:
uninitialized-storage - Severity:
High - Confidence:
High
An uninitialized storage variable will act as a reference to the first state variable, and can override a critical variable.
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 overridden to 0.
Initialize all storage variables.
- Check:
unprotected-upgrade - Severity:
High - Confidence:
High
Detects logic contract that can be destructed.
```solidity
contract Buggy is Initializable{
address payable owner;
function initialize() external initializer{
require(owner == address(0));
owner = msg.sender;
}
function kill() external{
require(msg.sender == owner);
selfdestruct(owner);
}
}
```
Buggy is an upgradeable contract. Anyone can call initialize on the logic contract, and destruct the contract.
Add a constructor to ensure initialize cannot be called on the logic contract.
- Check:
arbitrary-send - Severity:
High - Confidence:
Medium
Unprotected call to a function sending Ether to an arbitrary address.
contract ArbitrarySend{
address destination;
function setDestination(){
destination = msg.sender;
}
function withdraw() public{
destination.transfer(this.balance);
}
}Bob calls setDestination and withdraw. As a result he withdraws the contract's balance.
Ensure that an arbitrary user cannot withdraw unauthorized funds.
- Check:
controlled-delegatecall - Severity:
High - Confidence:
Medium
Delegatecall or callcode to an address controlled by the user.
contract Delegatecall{
function delegate(address to, bytes data){
to.delegatecall(data);
}
}Bob calls delegate and delegates the execution to his malicious contract. As a result, Bob withdraws the funds of the contract and destructs it.
Avoid using delegatecall. Use only trusted destinations.
- Check:
reentrancy-eth - Severity:
High - Confidence:
Medium
Detection of the reentrancy bug.
Do not report reentrancies that don't involve Ether (see reentrancy-no-eth)
function withdrawBalance(){
// send userBalance[msg.sender] Ether to msg.sender
// if mgs.sender is a contract, it will call its fallback function
if( ! (msg.sender.call.value(userBalance[msg.sender])() ) ){
throw;
}
userBalance[msg.sender] = 0;
}Bob uses the re-entrancy bug to call withdrawBalance two times, and withdraw more than its initial deposit to the contract.
Apply the check-effects-interactions pattern.
- Check:
erc20-interface - Severity:
Medium - Confidence:
High
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.
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.
Set the appropriate return values and types for the defined ERC20 functions.
- Check:
erc721-interface - Severity:
Medium - Confidence:
High
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.
contract Token{
function ownerOf(uint256 _tokenId) external view returns (bool);
//...
}Token.ownerOf does not return an address like 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.
Set the appropriate return values and vtypes for the defined ERC721 functions.
- Check:
incorrect-equality - Severity:
Medium - Confidence:
High
Use of strict equalities that can be easily manipulated by an attacker.
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.
Don't use strict equality to determine if an account has enough Ether or tokens.
- Check:
locked-ether - Severity:
Medium - Confidence:
High
Contract with a payable function, but without a withdrawal capacity.
pragma solidity 0.4.24;
contract Locked{
function receive() payable public{
}
}Every Ether sent to Locked will be lost.
Remove the payable attribute or add a withdraw function.
- Check:
shadowing-abstract - Severity:
Medium - Confidence:
High
Detection of state variables shadowed from abstract contracts.
contract BaseContract{
address owner;
}
contract DerivedContract is BaseContract{
address owner;
}owner of BaseContract is shadowed in DerivedContract.
Remove the state variable shadowing.
- Check:
tautology - Severity:
Medium - Confidence:
High
Detects expressions that are tautologies or contradictions.
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 a uint256, so x >= 0 will be always true.
y is a uint8, so y <512 will be always true.
Fix the incorrect comparison by changing the value type or the comparison.
- Check:
boolean-cst - Severity:
Medium - Confidence:
Medium
Detects the misuse of a Boolean constant.
contract A {
function f(uint x) public {
// ...
if (false) { // bad!
// ...
}
// ...
}
function g(bool b) public returns (bool) {
// ...
return (b || true); // bad!
// ...
}
}Boolean constants in code have only a few legitimate uses. Other uses (in complex expressions, as conditionals) indicate either an error or, most likely, the persistence of faulty code.
Verify and simplify the condition.
- Check:
constant-function-asm - Severity:
Medium - Confidence:
Medium
Functions declared as constant/pure/view using assembly code.
constant/pure/view was not enforced prior to Solidity 0.5.
Starting from Solidity 0.5, a call to a constant/pure/view function uses the STATICCALL opcode, which reverts in case of state modification.
As a result, a call to an incorrectly labeled function may trap a contract compiled with Solidity 0.5.
contract Constant{
uint counter;
function get() public view returns(uint){
counter = counter +1;
return counter
}
}Constant was deployed with Solidity 0.4.25. Bob writes a smart contract that interacts with Constant in Solidity 0.5.0.
All the calls to get revert, breaking Bob's smart contract execution.
Ensure the attributes of contracts compiled prior to Solidity 0.5.0 are correct.
- Check:
constant-function-state - Severity:
Medium - Confidence:
Medium
Functions declared as constant/pure/view change the state.
constant/pure/view was not enforced prior to Solidity 0.5.
Starting from Solidity 0.5, a call to a constant/pure/view function uses the STATICCALL opcode, which reverts in case of state modification.
As a result, a call to an incorrectly labeled function may trap a contract compiled with Solidity 0.5.
contract Constant{
uint counter;
function get() public view returns(uint){
counter = counter +1;
return counter
}
}Constant was deployed with Solidity 0.4.25. Bob writes a smart contract that interacts with Constant in Solidity 0.5.0.
All the calls to get revert, breaking Bob's smart contract execution.
Ensure that attributes of contracts compiled prior to Solidity 0.5.0 are correct.
- Check:
divide-before-multiply - Severity:
Medium - Confidence:
Medium
Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.
contract A {
function f(uint n) public {
coins = (oldSupply / n) * interest;
}
}If n is greater than oldSupply, coins will be zero. For example, with oldSupply = 5; n = 10, interest = 2, coins will be zero.
If (oldSupply * interest / n) was used, coins would have been 1.
In general, it's usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous.
Consider ordering multiplication before division.
- Check:
reentrancy-no-eth - Severity:
Medium - Confidence:
Medium
Detection of the reentrancy bug.
Do not report reentrancies that involve Ether (see reentrancy-eth).
function bug(){
require(not_called);
if( ! (msg.sender.call() ) ){
throw;
}
not_called = False;
} Apply the check-effects-interactions pattern.
- Check:
tx-origin - Severity:
Medium - Confidence:
Medium
tx.origin-based protection can be abused by a malicious contract if a legitimate user interacts with the malicious contract.
contract TxOrigin {
address owner = msg.sender;
function bug() {
require(tx.origin == owner);
}Bob is the owner of TxOrigin. Bob calls Eve's contract. Eve's contract calls TxOrigin and bypasses the tx.origin protection.
Do not use tx.origin for authorization.
- Check:
unchecked-lowlevel - Severity:
Medium - Confidence:
Medium
The return value of a low-level call is not checked.
contract MyConc{
function my_func(address payable dst) public payable{
dst.call.value(msg.value)("");
}
}The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls.
Ensure that the return value of a low-level call is checked or logged.
- Check:
unchecked-send - Severity:
Medium - Confidence:
Medium
The return value of a send is not checked.
contract MyConc{
function my_func(address payable dst) public payable{
dst.send(msg.value);
}
}The return value of send is not checked, so if the send fails, the Ether will be locked in the contract.
If send is used to prevent blocking operations, consider logging the failed send.
Ensure that the return value of send is checked or logged.
- Check:
uninitialized-local - Severity:
Medium - Confidence:
Medium
Uninitialized local variables.
contract Uninitialized is Owner{
function withdraw() payable public onlyOwner{
address to;
to.transfer(this.balance)
}
}Bob calls transfer. As a result, all Ether is sent to the address 0x0 and is lost.
Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero.
- Check:
unused-return - Severity:
Medium - Confidence:
Medium
The return value of an external call is not stored in a local or state variable.
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.
Ensure that all the return values of the function calls are used.
- Check:
shadowing-builtin - Severity:
Low - Confidence:
High
Detection of shadowing built-in symbols using local variables, state variables, functions, modifiers, or events.
pragma solidity ^0.4.24;
contract Bug {
uint now; // Overshadows current time stamp.
function assert(bool condition) public {
// Overshadows built-in symbol for providing assertions.
}
function get_next_expiration(uint earlier_time) private returns (uint) {
return now + 259200; // References overshadowed timestamp.
}
}now is defined as a state variable, and shadows with the built-in symbol now. The function assert overshadows the built-in assert function. Any use of either of these built-in symbols may lead to unexpected results.
Rename the local variables, state variables, functions, modifiers, and events that shadow a builtin symbol.
- Check:
shadowing-local - Severity:
Low - Confidence:
High
Detection of shadowing using local variables.
pragma solidity ^0.4.24;
contract Bug {
uint owner;
function sensitive_function(address owner) public {
// ...
require(owner == msg.sender);
}
function alternate_sensitive_function() public {
address owner = msg.sender;
// ...
require(owner == msg.sender);
}
}sensitive_function.owner shadows Bug.owner. As a result, the use of owner in sensitive_function might be incorrect.
Rename the local variables that shadow another component.
- Check:
void-cst - Severity:
Low - Confidence:
High
Detect the call to a constructor that is not implemented
contract A{}
contract B is A{
constructor() public A(){}
}When reading B's constructor definition, we might assume that A() initiates the contract, but no code is executed.
Remove the constructor call.
- Check:
calls-loop - Severity:
Low - Confidence:
Medium
Calls inside a loop might lead to a denial-of-service attack.
contract CallsInLoop{
address[] destinations;
constructor(address[] newDestinations) public{
destinations = newDestinations;
}
function bad() external{
for (uint i=0; i < destinations.length; i++){
destinations[i].transfer(i);
}
}
}If one of the destinations has a fallback function that reverts, bad will always revert.
Favor pull over push strategy for external calls.
- Check:
reentrancy-benign - Severity:
Low - Confidence:
Medium
Detection of the reentrancy bug.
Only report reentrancy that acts as a double call (see reentrancy-eth, reentrancy-no-eth).
function callme(){
if( ! (msg.sender.call()() ) ){
throw;
}
counter += 1
} callme contains a reentrancy. The reentrancy is benign because it's exploitation would have the same effect as two consecutive calls.
Apply the check-effects-interactions pattern.
- Check:
reentrancy-events - Severity:
Low - Confidence:
Medium
Detection of the reentrancy bug. Only report reentrancies leading to out-of-order events.
function bug(Called d){
counter += 1;
d.f();
emit Counter(counter);
}If d.() re-enters, the Counter events will be shown in an incorrect order, which might lead to issues for third parties.
Apply the check-effects-interactions pattern.
- Check:
timestamp - Severity:
Low - Confidence:
Medium
Dangerous usage of block.timestamp. block.timestamp can be manipulated by miners.
"Bob's contract relies on block.timestamp for its randomness. Eve is a miner and manipulates block.timestamp to exploit Bob's contract.
Avoid relying on block.timestamp.
- Check:
assembly - Severity:
Informational - Confidence:
High
The use of assembly is error-prone and should be avoided.
Do not use evm assembly.
- Check:
boolean-equal - Severity:
Informational - Confidence:
High
Detects the comparison to boolean constants.
contract A {
function f(bool x) public {
// ...
if (x == true) { // bad!
// ...
}
// ...
}
}Boolean constants can be used directly and do not need to be compare to true or false.
Remove the equality to the boolean constant.
- Check:
deprecated-standards - Severity:
Informational - Confidence:
High
Detect the usage of deprecated standards.
contract ContractWithDeprecatedReferences {
// Deprecated: Change block.blockhash() -> blockhash()
bytes32 globalBlockHash = block.blockhash(0);
// Deprecated: Change constant -> view
function functionWithDeprecatedThrow() public constant {
// Deprecated: Change msg.gas -> gasleft()
if(msg.gas == msg.value) {
// Deprecated: Change throw -> revert()
throw;
}
}
// Deprecated: Change constant -> view
function functionWithDeprecatedReferences() public constant {
// Deprecated: Change sha3() -> keccak256()
bytes32 sha3Result = sha3("test deprecated sha3 usage");
// Deprecated: Change callcode() -> delegatecall()
address(this).callcode();
// Deprecated: Change suicide() -> selfdestruct()
suicide(address(0));
}
}Replace all uses of deprecated symbols.
- Check:
erc20-indexed - Severity:
Informational - Confidence:
High
Detects whether events defined by the ERC20 specification that should have some parameters as indexed are missing the indexed keyword.
contract ERC20Bad {
// ...
event Transfer(address from, address to, uint value);
event Approval(address owner, address spender, uint value);
// ...
}Transfer and Approval events should have the 'indexed' keyword on their two first parameters, as defined by the ERC20 specification.
Failure to include these keywords will exclude the parameter data in the transaction/block's bloom filter, so external tooling searching for these parameters may overlook them and fail to index logs from this token contract.
Add the indexed keyword to event parameters that should include it, according to the ERC20 specification.
- Check:
low-level-calls - Severity:
Informational - Confidence:
High
The use of low-level calls is error-prone. Low-level calls do not check for code existence or call success.
Avoid low-level calls. Check the call success. If the call is meant for a contract, check for code existence.
- Check:
naming-convention - Severity:
Informational - Confidence:
High
Solidity defines a naming convention that should be followed.
- Allow constant variable name/symbol/decimals to be lowercase (
ERC20). - Allow
_at the beginning of themixed_casematch for private variables and unused parameters.
Follow the Solidity naming convention.
- Check:
pragma - Severity:
Informational - Confidence:
High
Detect whether different Solidity versions are used.
Use one Solidity version.
- Check:
solc-version - Severity:
Informational - Confidence:
High
solc frequently releases new compiler versions. Using an old version prevents access to new Solidity security checks.
We also recommend avoiding complex pragma statement.
Deploy with any of the following Solidity versions:
- 0.5.11 - 0.5.13,
- 0.5.15 - 0.5.17,
- 0.6.8,
- 0.6.10 - 0.6.11. Use a simple pragma version that allows any of these versions. Consider using the latest version of Solidity for testing.
- Check:
unused-state - Severity:
Informational - Confidence:
High
Unused state variable.
Remove unused state variables.
- Check:
reentrancy-unlimited-gas - Severity:
Informational - Confidence:
Medium
Detection of the reentrancy bug.
Only report reentrancy that is based on transfer or send.
function callme(){
msg.sender.transfer(balances[msg.sender]):
balances[msg.sender] = 0;
} send and transfer do not protect from reentrancies in case of gas price changes.
Apply the check-effects-interactions pattern.
- Check:
too-many-digits - Severity:
Informational - Confidence:
Medium
Literals with many digits are difficult to read and review.
contract MyContract{
uint 1_ether = 10000000000000000000;
}While 1_ether looks like 1 ether, it is 10 ether. As a result, it's likely to be used incorrectly.
Use:
- Check:
constable-states - Severity:
Optimization - Confidence:
High
Constant state variables should be declared constant to save gas.
Add the constant attributes to state variables that never change.
- Check:
external-function - Severity:
Optimization - Confidence:
High
public functions that are never called by the contract should be declared external to save gas.
Use the external attribute for functions never called from the contract.