-
Notifications
You must be signed in to change notification settings - Fork 980
Detector Documentation
List of public detectors
- 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 ethers 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 uinitialized 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 override to 0.
Initialize all the storage variables.
- Check:
arbitrary-send - Severity:
High - Confidence:
Medium
Unprotected call to a function executing sending ethers 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 unauthorize 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 its 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 re-entrancy bug.
Do not report reentrancies that don't involve ethers (see reentrancy-no-eth)
function withdrawBalance(){
// send userBalance[msg.sender] ethers 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 value-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 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.
Set the appropriate return values and value-types 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 ethers or tokens.
- Check:
locked-ether - Severity:
Medium - Confidence:
High
Contract with a payable function, but without a withdraw 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:
constant-function-asm - Severity:
Medium - Confidence:
Medium
Functions declared as constant/pure/view using assembly code.
constant/pure/view was not enforced prior 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 interacting with Constant in Solidity 0.5.0.
All the calls to get revert, breaking Bob's smart contract execution.
Ensure that 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 changing the state.
constant/pure/view was not enforced prior 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 interacting with Constant in Solidity 0.5.0.
All the calls to get revert, breaking Bob's smart contract execution.
Ensure that the attributes of contracts compiled prior to Solidity 0.5.0 are correct.
- Check:
reentrancy-no-eth - Severity:
Medium - Confidence:
Medium
Detection of the re-entrancy bug.
Do not report reentrancies that involve ethers (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 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. As a result if the callfailed, 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 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. As a result if the send failed, the ether will be locked in the contract.
If send is used to prevent blocking operations, consider logging the failed sent.
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, the ethers 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:
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/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 variable/state variable/function/modifier/event, so as not to mistakenly overshadow any built-in symbol definitions.
- 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 inside sensitive_function might be incorrect.
Rename the local variable so as not to mistakenly overshadow any state variable/function/modifier/event definitions.
- Check:
void-cst - Severity:
Low - Confidence:
High
Detect the call to a constructor not implemented
contract A{}
contract B is A{
constructor() public A(){}
}By reading B's constructor definition, the reader might assume that A() initiate the contract, while no code is executed.
Remove the constructor call.
- Check:
calls-loop - Severity:
Low - Confidence:
Medium
Calls inside a loop might lead to 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 which reverts, bad will always revert.
Favor pull over push strategy for external calls.
- Check:
reentrancy-benign - Severity:
Low - Confidence:
Medium
Detection of the re-entrancy 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 re-entrancy bug. Only report reentrancies leading to out-of-order Events
function bug(Called d){
counter += 1;
d.f();
emit Counter(counter);
}If d.() reenters, the Counter events will be showed 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:
deprecated-standards - Severity:
Informational - Confidence:
High
Detect the usage of deprecated standards (as defined by SWC-111), excluding only constant keyword detection on functions.
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 that events defined by the ERC20 specification which are meant to 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);
// ...
}In this case, 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 not include the parameter data in the transaction/block's bloom filter. This may cause external tooling searching for these parameters to overlook them, and fail to index logs from this token contract.
Add the indexed keyword to event parameters which 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 variables name/symbol/decimals to be lowercase (ERC20)
- Allow
_at the beginning of the mixed_case match for private variables and unused parameters.
Follow the Solidity naming convention.
- Check:
pragma - Severity:
Informational - Confidence:
High
Detect if 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 recommend avoiding complex pragma statement.
Use Solidity 0.4.25 or 0.5.11. Consider using the latest version of Solidity for testing the compilation, and a trusted version for deploying.
- 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 re-entrancy bug.
Only report reentrancy that are based on transfer or send.
function callme(){
msg.sender.transfer(balances[msg.sender]):
balances[msg.sender] = 0;
} send and transfer does not protect from reentrancies in case of gas-price change.
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, its usage is likely to be incorrect.
Use:
- Check:
constable-states - Severity:
Optimization - Confidence:
High
Constant state variable should be declared constant to save gas.
Add the constant attributes to the 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.