-
Notifications
You must be signed in to change notification settings - Fork 1k
Detector Documentation
List of public detectors
- Check:
abiencoderv2-array - Severity:
High - Confidence:
High
solc versions 0.4.7-0.5.9 contain a compiler bug leading to incorrect ABI encoder usage.
contract A {
uint[2][3] bad_arr = [[1, 2], [3, 4], [5, 6]];
/* Array of arrays passed to abi.encode is vulnerable */
function bad() public {
bytes memory b = abi.encode(bad_arr);
}
}abi.encode(bad_arr) in a call to bad() will incorrectly encode the array as [[1, 2], [2, 3], [3, 4]] and lead to unintended behavior.
Use a compiler >= 0.5.10.
- Check:
arbitrary-send-erc20 - Severity:
High - Confidence:
High
Detect when msg.sender is not used as from in transferFrom.
function a(address from, address to, uint256 amount) public {
erc20.transferFrom(from, to, am);
}Alice approves this contract to spend her ERC20 tokens. Bob can call a and specify Alice's address as the from parameter in transferFrom, allowing him to transfer Alice's tokens to himself.
Use msg.sender as from in transferFrom.
- Check:
array-by-reference - Severity:
High - Confidence:
High
Detect arrays passed to a function that expects reference to a storage array
contract Memory {
uint[1] public x; // storage
function f() public {
f1(x); // update x
f2(x); // do not update x
}
function f1(uint[1] storage arr) internal { // by reference
arr[0] = 1;
}
function f2(uint[1] arr) internal { // by value
arr[0] = 2;
}
}Bob calls f(). Bob assumes that at the end of the call x[0] is 2, but it is 1.
As a result, Bob's usage of the contract is incorrect.
Ensure the correct usage of memory and storage in the function parameters. Make all the locations explicit.
- Check:
blockhash-current - Severity:
High - Confidence:
High
blockhash on the current block will always return 0.
contract Seed{
bytes32 public seed = sha3(blockhash(block.number));
} blockhash(block.number) will always be zero, so Seed wil always be the same value .
Use blockhash(block.number - 1), and check that its return value is not zero.
- Check:
encodePacked - Severity:
High - Confidence:
High
Detect collision due to dynamic type usages in abi.encodePacked
contract Sign {
function get_hash_for_signature(string name, string doc) external returns(bytes32) {
return keccak256(abi.encodePacked(name, doc));
}
}Bob calls get_hash_for_signature with (bob, This is the content). The hash returned is used as an ID.
Eve creates a collision with the ID using (bo, bThis is the content) and compromises the system.
Do not use more than one dynamic type in abi.encodePacked()
(see the Solidity documentation).
Use abi.encode(), preferably.
- Check:
incorrect-shift - Severity:
High - Confidence:
High
Detect if the values in a shift operation are reversed
contract C {
function f() internal returns (uint a) {
assembly {
a := shr(a, 8)
}
}
}The shift statement will right-shift the constant 8 by a bits
Swap the order of parameters.
- Check:
missing-return - Severity:
High - Confidence:
High
Function with missing return statements.
function check(uint a) external returns(bool){
require(a >= 10);
}f always return false.
Make all return statements explicit and initiate all the return variables.
- Check:
multiple-constructors - Severity:
High - Confidence:
High
Detect multiple constructor definitions in the same contract (using new and old schemes).
contract A {
uint x;
constructor() public {
x = 0;
}
function A() public {
x = 1;
}
function test() public returns(uint) {
return x;
}
}In Solidity 0.4.22, a contract with both constructor schemes will compile. The first constructor will take precedence over the second, which may be unintended.
Only declare one constructor, preferably using the new scheme constructor(...) instead of function <contractName>(...).
- 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:
protected-vars - Severity:
High - Confidence:
High
Detect unprotected variable that are marked protected
contract Buggy{
/// @custom:security write-protection="onlyOwner()"
address owner;
function set_protected() public onlyOwner(){
owner = msg.sender;
}
function set_not_protected() public{
owner = msg.sender;
}
} owner must be always written by function using onlyOwner (write-protection="onlyOwner()"), however anyone can call set_not_protected.
Add access controls to the vulnerable function
- Check:
public-mappings-nested - Severity:
High - Confidence:
High
Prior to Solidity 0.5, a public mapping with nested structures returned incorrect values.
Bob interacts with a contract that has a public mapping with nested structures. The values returned by the mapping are incorrect, breaking Bob's usage
Do not use public mapping with nested structures.
- 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 to improve code readability.
- 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-erc20-permit - Severity:
High - Confidence:
Medium
Detect when msg.sender is not used as from in transferFrom and permit is used.
function bad(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) public {
erc20.permit(from, address(this), value, deadline, v, r, s);
erc20.transferFrom(from, to, value);
}If an ERC20 token does not implement permit and has a fallback function e.g. WETH, transferFrom allows an attacker to transfer all tokens approved for this contract.
Ensure that the underlying ERC20 token correctly implements a permit function.
- Check:
arbitrary-send-eth - Severity:
High - Confidence:
Medium
Unprotected call to a function sending Ether to an arbitrary address.
contract ArbitrarySendEth{
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-array-length - Severity:
High - Confidence:
Medium
Detects the direct assignment of an array's length.
contract A {
uint[] testArray; // dynamic size array
function f(uint usersCount) public {
// ...
testArray.length = usersCount;
// ...
}
function g(uint userIndex, uint val) public {
// ...
testArray[userIndex] = val;
// ...
}
}Contract storage/state-variables are indexed by a 256-bit integer.
The user can set the array length to 2**256-1 in order to index all storage slots.
In the example above, one could call the function f to set the array length, then call the function g to control any storage slot desired.
Note that storage slots here are indexed via a hash of the indexers; nonetheless, all storage will still be accessible and could be controlled by the attacker.
Do not allow array lengths to be set directly set; instead, opt to add values as needed. Otherwise, thoroughly review the contract to ensure a user-controlled variable cannot reach an array length assignment.
- 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:
delegatecall-loop - Severity:
High - Confidence:
Medium
Detect the use of delegatecall inside a loop in a payable function.
contract DelegatecallInLoop{
mapping (address => uint256) balances;
function bad(address[] memory receivers) public payable {
for (uint256 i = 0; i < receivers.length; i++) {
address(this).delegatecall(abi.encodeWithSignature("addBalance(address)", receivers[i]));
}
}
function addBalance(address a) public payable {
balances[a] += msg.value;
}
}When calling bad the same msg.value amount will be accredited multiple times.
Carefully check that the function called by delegatecall is not payable/doesn't use msg.value.
- Check:
function-visibility - Severity:
High - Confidence:
Medium
Detect functions with leading underscore that should be internal.
contract Bug{
address owner = msg.sender;
function _setOwner(address addr) external{
owner = addr;
}
modifier onlyOwner(){
require(owner == msg.sender);
_;
}
}Alice deploys the contract. Bob calls _setOwner and becomes the owner of the contract.
Protect functions writing to sensitive state variables.
- Check:
incorrect-exp - Severity:
High - Confidence:
Medium
Detect use of bitwise xor ^ instead of exponential **
contract Bug{
uint UINT_MAX = 2^256 - 1;
...
}Alice deploys a contract in which UINT_MAX incorrectly uses ^ operator instead of ** for exponentiation
Use the correct operator ** for exponentiation.
- Check:
incorrect-return - Severity:
High - Confidence:
Medium
Detect if a return is used where a leave should be used.
contract C {
function f() internal returns (uint a, uint b) {
assembly {
return (5, 6)
}
}
function g() returns (bool){
f();
return true;
}
}The return statement in f will cause execution in g to halt.
The function will return 6 bytes starting from offset 5, instead of returning a boolean.
Use the leave statement.
- Check:
incorrect-sender - Severity:
High - Confidence:
Medium
Detect incorrect check on msg.sender.
contract Wallet {
address walletAddress;
modifier onlyFromWallet {
require(msg.sender != walletAddress);
_;
}
constructor () public {
walletAddress = msg.sender;
}
function withdraw() onlyFromWallet {
msg.sender.transfer(bonus);
}
}onlyFromWallet incorrectly checks msg.sender. As a result anyone can withdraw the wallet's funds.
Fix the incorrect access control.
- Check:
missing-constructor - Severity:
High - Confidence:
Medium
Detection of a contract that contains sensitive variables without a constructor to initiate them.
contract Owner{
address owner;
function transferOwnership(address newOwner) external{
require(owner == msg.sender);
owner = newOwner;
}
}owner is not initialized at construction. As a result it is always 0 and cannot be changed.
Initiate the sensitive variables in the constructor.
- Check:
modifier-like-keyword - Severity:
High - Confidence:
Medium
Detection of a contract that contains modifier that looks similar to Solidity keyword
contract Contract{
modifier pub1ic() {
}
function doSomething() pub1ic {
require(owner == msg.sender);
owner = newOwner;
}
}pub1ic is a modifier meant to look like a Solidity keyword
Rename the modifier.
- Check:
msg-value-loop - Severity:
High - Confidence:
Medium
Detect the use of msg.value inside a loop.
contract MsgValueInLoop{
mapping (address => uint256) balances;
function bad(address[] memory receivers) public payable {
for (uint256 i=0; i < receivers.length; i++) {
balances[receivers[i]] += msg.value;
}
}
}Track msg.value through a local variable and decrease its amount on every iteration/usage.
- Check:
overflow-erc20-balance - Severity:
High - Confidence:
Medium
ERC20 balance is modified without checking for overflow.
function transfer(address to, uint256 value) external returns (bool){
balanceOf[msg.sender] -= value;
balanceOf[to] += value;
return true;
}Alice has 100 tokens. She transfers 101 tokens to Bob. As a result, Alice has an infinite amount of tokens.
Use SafeMath. If SafeMath is not used, check manually for overflow.
- Check:
race-condition-ecrecover - Severity:
High - Confidence:
Medium
Detect the incorrect usage of ecrecover that leads to a race condition.
mapping(address => bool) valid_signer;
function bad(bytes code, uint8 recoveryByte, bytes32 ecdsaR, bytes32 ecdsaS){
bytes32 msghash = keccak256(code);
address signer = ecrecover(msghash, recoveryByte, ecdsaR, ecdsaS);
require(signer!=0);
require(valid_signer[signer]);
msg.sender.transfer(this.balance);
}Alice calls bad. Bob sees Alice's transaction before it has been accepted and calls bad with the same parameters. As a result, Bob can withdraw the contract's balance.
Protect ecrecover against race conditions.
In the previous example, you can build msghash with the msg.sender value:
bytes32 msghash = keccak256(msg.sender, code);- Check:
race-condition-init - Severity:
High - Confidence:
Medium
Detect race condition at contract's initialization.
pragma solidity ^0.4.24;
contract Bug{
bool was_init = false;
address owner;
constructor(){}
function init() not_init{
was_init = true;
owner = msg.sender;
}
modifier onlyOwner(){
require(msg.sender == owner);
_;
}
modifier not_init(){
require(!was_init);
_;
}
}Alice deploys Bug and calls init().
Bob sees Alice's transaction before it has been accepted and calls init().
Bob's transaction is accepted before Alice's one, so Bob becomes the owner of the contract.
Call the initialization function in the contract's constructor.
Upgradable contracts using the delegatecall proxy pattern need an initialization function.
Ensure that your deployment script is robust against race conditions during the contract upgrade.
- 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:
storage-array - Severity:
High - Confidence:
Medium
solc versions 0.4.7-0.5.10 contain a compiler bug
leading to incorrect values in signed integer arrays.
contract A {
int[3] ether_balances; // storage signed integer array
function bad0() private {
// ...
ether_balances = [-1, -1, -1];
// ...
}
}bad0() uses a (storage-allocated) signed integer array state variable to store the ether balances of three accounts.
-1 is supposed to indicate uninitialized values but the Solidity bug makes these as 1, which could be exploited by the accounts.
Use a compiler version >= 0.5.10.
- Check:
unchecked-transfer - Severity:
High - Confidence:
Medium
The return value of an external transfer/transferFrom call is not checked
contract Token {
function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
}
contract MyBank{
mapping(address => uint) balances;
Token token;
function deposit(uint amount) public{
token.transferFrom(msg.sender, address(this), amount);
balances[msg.sender] += amount;
}
}Several tokens do not revert in case of failure and return false. If one of these tokens is used in MyBank, deposit will not revert if the transfer fails, and an attacker can call deposit for free..
Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.
- Check:
unprotected-function - Severity:
High - Confidence:
Medium
Detect functions that should be protected.
contract Bug{
address owner = msg.sender;
function setOwner() external{
owner = msg.sender;
}
modifier isOwner(){
require(owner == msg.sender);
_;
}
}Alice deploys the contract. Bob calls setOwner and becomes the owner of the contract.
Protect functions writing to sensitive state variables.
- Check:
weak-prng - Severity:
High - Confidence:
Medium
Weak PRNG due to a modulo on block.timestamp, now or blockhash. These can be influenced by miners to some extent so they should be avoided.
contract Game {
uint reward_determining_number;
function guessing() external{
reward_determining_number = uint256(block.blockhash(10000)) % 10;
}
}Eve is a miner. Eve calls guessing and re-orders the block containing the transaction.
As a result, Eve wins the game.
Do not use block.timestamp, now or blockhash as a source of randomness
- Check:
enum-conversion - Severity:
Medium - Confidence:
High
Detect out-of-range enum conversion (solc < 0.4.5).
pragma solidity 0.4.2;
contract Test{
enum E{a}
function bug(uint a) public returns(E){
return E(a);
}
}Attackers can trigger unexpected behaviour by calling bug(1).
Use a recent compiler version. If solc <0.4.5 is required, check the enum conversion range.
- 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:
iscontract-modifier - Severity:
Medium - Confidence:
High
Detect any modifier definitions matching name [is|only]Contract that do not read tx.origin.
contract A {
modifier isContract {
uint32 size;
address a = msg.sender;
assembly {
size := extcodesize(a)
}
require (size > 0);
_;
}
}Using extcodesize to detect if the caller is a contract is flawed, as a contract mid-deployment will not yet have its code set, and can trick the vulnerable contract.
Check msg.sender != tx.origin to verify the caller is a contract.
- 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:
mapping-deletion - Severity:
Medium - Confidence:
High
A deletion in a structure containing a mapping will not delete the mapping (see the Solidity documentation). The remaining data may be used to compromise the contract.
struct BalancesStruct{
address owner;
mapping(address => uint) balances;
}
mapping(address => BalancesStruct) public stackBalance;
function remove() internal{
delete stackBalance[msg.sender];
}remove deletes an item of stackBalance.
The mapping balances is never deleted, so remove does not work as intended.
Use a lock mechanism instead of a deletion to disable structure containing a mapping.
- 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:
tautological-compare - Severity:
Medium - Confidence:
High
A variable compared to itself is probably an error as it will always return true for ==, >=, <= and always false for <, > and !=.
function check(uint a) external returns(bool){
return (a >= a);
}check always return true.
Remove comparison or compare to different value.
- 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:
write-after-write - Severity:
Medium - Confidence:
High
Detects variables that are written but never read and written again.
```solidity
contract Buggy{
function my_func() external initializer{
// ...
a = b;
a = c;
// ..
}
}
```
`a` is first asigned to `b`, and then to `c`. As a result the first write does nothing.
Fix or remove the writes.
- Check:
balance-value - Severity:
Medium - Confidence:
Medium
this.balance will include the value sent by msg.value, which might lead to incorrect computation.
contract Bug{
function buy() public payable{
uint minted = msg.value * (1000 / address(this).balance);
// ...
}
}buy is meant to compute a price that changes a ratio over the contract's balance.
.balance will include msg.value and lead to an incorrect price computation.
Subtract msg.value from the balance.
- 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:
controlled-loop - Severity:
Medium - Confidence:
Medium
Loop with a user-controlled/tainted iteration count.
contract C{
uint n;
function set(uint _n) public{
n = _n;
}
function bad() public{
uint i;
uint counter;
for(i=0; i<n; i++){
counter = i;
}
}
}The loop iteration counter length n in function bad() is a state variable controlled/tainted by any user who calls the public function set().
If n is set very high, it could lead to DoS and high gas usage
Avoid user-controlled/tainted loop iteration or use a state machine to allow the loop iteration through multiple transactions.
- Check:
controlled-lowlevelcall - Severity:
Medium - Confidence:
Medium
Low-level call with a user-controlled data field.
address token;
function call_token(bytes data){
token.call(data);
}token points to an ERC20 token. Bob uses call_token to call the transfer function of token to withdraw all tokens held by the contract.
Avoid low-level calls. Consider using a whitelist of function ID to call.
- 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:
empty-functions - Severity:
Medium - Confidence:
Medium
Detect functions with an empty body.
contract BaseContract{
function f1() external returns(uint){}
function f2() external returns(uint){}
}
contract DerivedContract is BaseContract{
function f1() external returns(uint){
return 42;
}
}DerivedContract does not overide f2. As a result, f2 will always return 0.
Do not declare functions with an empty body. Favor interface over abstract contract.
- Check:
incorrect-transferFrom - Severity:
Medium - Confidence:
Medium
transferFrom is not using msg.sender to decrease the allowance.
function transferFrom(address from, address to, uint256 value) external returns (bool){
require(balanceOf[from] >= value);
require(allowance[from] >= value);
balanceOf[from] -= value;
balanceOf[to] -= value;
return true;
}transferFrom decreases the allowance of from instead of the msg.sender. As a result, anyone can transfer the tokens to the destination.
Use msg.sender in transferFrom
- Check:
msg-value - Severity:
Medium - Confidence:
Medium
If a function uses msg.value it should be payable. msg.value will always be 0 otherwise.
pragma solidity ^0.4.24;
contract NonPayable{
mapping(address => uint) balances;
function buy() external{
balances[msg.sender] += msg.value;
}
}buy is not payable, so nobody can buy tokens.
Add the payable attribute to the functions using msg.value.
- Check:
overflow-erc20-allowance - Severity:
Medium - Confidence:
Medium
ERC20 allowance is modified without checking for overflow.
function buggyDecreaseApproval(address spender, uint value) external{
allowance[msg.sender][spender] -= value;
}buggyDecreaseApproval decreases the allowance without checking for overflow.
Alice approves Bob's 100 tokens.
Alice wants to reduce the allowance of 10 and call buggyDecreaseApproval with a value of 10, but Bob has already spent the 100 tokens.
As a result, Bob has a large number for the allowance.
Use SafeMath, or check manually for overflow.
- Check:
pausable - Severity:
Medium - Confidence:
Medium
ERC20 balance/allowance is modified without whenNotPaused modifier.
function buggyTransfer(address to, uint256 value) external returns (bool){
balanceOf[msg.sender] -= value;
balanceOf[to] += value;
return true;
}buggyTransfer performs a token transfer but does not use Pausable's whenNotPaused modifier.
If the token admin/owner pauses the ERC20 contract to trigger an emergency stop, it will not apply to this function.
This results in Txs transferring even in a paused state, which corrupts the contract balance state and affects recovery.
When using Pausable, apply whenNotPaused modifier to all relevant functions.
- 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:
reused-constructor - Severity:
Medium - Confidence:
Medium
Detects if the same base constructor is called with arguments from two different locations in the same inheritance hierarchy.
pragma solidity ^0.4.0;
contract A{
uint num = 5;
constructor(uint x) public{
num += x;
}
}
contract B is A{
constructor() A(2) public { /* ... */ }
}
contract C is A {
constructor() A(3) public { /* ... */ }
}
contract D is B, C {
constructor() public { /* ... */ }
}
contract E is B {
constructor() A(1) public { /* ... */ }
}The constructor of A is called multiple times in D and E:
-
Dinherits fromBandC, both of which constructA. -
Eonly inherits fromB, butBandEconstructA. .
Remove the duplicate constructor call.
- Check:
tx-gasprice - Severity:
Medium - Confidence:
Medium
Sunce tx.gasprice can be set to zero, any operation on it must handle this value.
contract TxGasprice {
uint256 fee_multiplier;
function compute_price() returns (uint){
return fee_multiplier * tx.gasprice;
}Eve is a miner. Eve calls TxGasprice and sets tx.gasprice to zero, so he does not pay the fee.
Check tx.gasprice for zero value.
- 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-blockhash - Severity:
Medium - Confidence:
Medium
blockhash can return 0 if its argument is not one of the 256 most recent blocks. As a result, blockhash must be checked for the zero value
contract UncheckedBlockhash {
uint reward_determining_number;
uint blocknumber;
uint number_of_participants;
function calc_reward_determining_number() internal payable {
reward_determining_number = uint256(block.blockhash(blocknumber)) % number_of_participants;
}
} Bob is the owner of UncheckedBlockhash which determines rewards using logic based on the blocknumber.
If the blocknumber used happens to be 256 behind the current block, the return value of blockhash is 0.
As a result, Bob can easily manipulate the game.
Check for the zero value after every call to blockash.
- 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 to improve code readability.
- Check:
uninitialized-return - Severity:
Medium - Confidence:
Medium
Function that can return uninitialized values.
function f(uint a, uint b) external returns(uint){
if(a>b){
return a;
}
}f has an implicit return statement: If a<=b, f returns 0 (the default value).
Make all return statements explicit and initiate all the return variables.
- Check:
unspecified-order - Severity:
Medium - Confidence:
Medium
Detect operations following an unspecified order.
contract Bug{
uint sv1= 0;
/* Modifies state variable sv1 */
function dup(uint x) private returns (uint) {
sv1 = 1;
return 2*x;
}
function bad0() public {
/* RHS uses sv1 which is modified by the call to dup() */
uint x = sv1 + dup(3);
...
}
}The operation order of sv1 + dup(3) and sv1 = 1 is unspecified in Solidity, so the result of bad0 execution might change with the Solidity version.
Split the operations so the expected order is clear.
- Check:
unused-event - Severity:
Medium - Confidence:
Medium
Events that are defined but not emitted.
contract C {
event C_event ();
}C has defined an event C_event but no function emits it.
Ensure that all events that are defined are emitted.
- 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:
unused-return-internal - Severity:
Medium - Confidence:
Medium
The return value of an internal call is not stored in a local or state variable.
contract MyConc{
function f() internal returns (bool) {
return true;
}
function my_func() public{
f()
}
}MyConc calls the internal f function, but does not use the return value from f.
Ensure that all the return values of internal function calls are used.
- Check:
use-after-delete - Severity:
Medium - Confidence:
Medium
Using values of variables after they have been explicitly deleted may lead to unexpected behavior or compromise.
mapping(address => uint) public balances;
function f() public {
delete balances[msg.sender];
msg.sender.transfer(balances[msg.sender]);
}balances[msg.sender] is deleted before it's sent to the caller, leading the transfer to always send zero.
Make sure deleted variables are not used later.
- Check:
incorrect-modifier - Severity:
Low - Confidence:
High
If a modifier does not execute _ or revert, the execution of the function will return the default value, which can be misleading for the caller.
modidfier myModif(){
if(..){
_;
}
}
function get() myModif returns(uint){
}If the condition in myModif is false, the execution of get() will return 0.
All the paths in a modifier must execute _ or revert.
- 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:
uninitialized-fptr-cst - Severity:
Low - Confidence:
High
solc versions 0.4.5-0.4.26 and 0.5.0-0.5.8 contain a compiler bug leading to unexpected behavior when calling uninitialized function pointers in constructors.
contract bad0 {
constructor() public {
/* Uninitialized function pointer */
function(uint256) internal returns(uint256) a;
a(10);
}
}The call to a(10) will lead to unexpected behavior because function pointer a is not initialized in the constructor.
Initialize function pointers before calling. Avoid function pointers if possible.
- Check:
variable-scope - Severity:
Low - Confidence:
High
Detects the possible usage of a variable before the declaration is stepped over (either because it is later declared, or declared in another scope).
contract C {
function f(uint z) public returns (uint) {
uint y = x + 9 + z; // 'z' is used pre-declaration
uint x = 7;
if (z % 2 == 0) {
uint max = 5;
// ...
}
// 'max' was intended to be 5, but it was mistakenly declared in a scope and not assigned (so it is zero).
for (uint i = 0; i < max; i++) {
x += 1;
}
return x;
}
}In the case above, the variable x is used before its declaration, which may result in unintended consequences.
Additionally, the for-loop uses the variable max, which is declared in a previous scope that may not always be reached. This could lead to unintended consequences if the user mistakenly uses a variable prior to any intended declaration assignment. It also may indicate that the user intended to reference a different variable.
Move all variable declarations prior to any usage of the variable, and ensure that reaching a variable declaration does not depend on some conditional if it is used unconditionally.
- 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:
events-access - Severity:
Low - Confidence:
Medium
Detect missing events for critical access control parameters
contract C {
modifier onlyAdmin {
if (msg.sender != owner) throw;
_;
}
function updateOwner(address newOwner) onlyAdmin external {
owner = newOwner;
}
}updateOwner() has no event, so it is difficult to track off-chain owner changes.
Emit an event for critical parameter changes.
- Check:
events-maths - Severity:
Low - Confidence:
Medium
Detect missing events for critical arithmetic parameters.
contract C {
modifier onlyOwner {
if (msg.sender != owner) throw;
_;
}
function setBuyPrice(uint256 newBuyPrice) onlyOwner public {
buyPrice = newBuyPrice;
}
function buy() external {
... // buyPrice is used to determine the number of tokens purchased
}
}updateOwner() has no event, so it is difficult to track off-chain changes in the buy price.
Emit an event for critical parameter changes.
- Check:
incorrect-unary - Severity:
Low - Confidence:
Medium
Unary expressions such as x=+1 probably typos.
contract Bug{
uint public counter;
function increase() public returns(uint){
counter=+1;
return counter;
}
}increase() uses =+ instead of +=, so counter will never exceed 1.
Remove the unary expression.
- Check:
missing-zero-check - Severity:
Low - Confidence:
Medium
Detect missing zero address validation.
contract C {
modifier onlyAdmin {
if (msg.sender != owner) throw;
_;
}
function updateOwner(address newOwner) onlyAdmin external {
owner = newOwner;
}
}Bob calls updateOwner without specifying the newOwner, so Bob loses ownership of the contract.
Check that the address is not zero.
- 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:
assert-state-change - Severity:
Informational - Confidence:
High
Incorrect use of assert(). See Solidity best practices.
contract A {
uint s_a;
function bad() public {
assert((s_a += 1) > 10);
}
}The assert in bad() increments the state variable s_a while checking for the condition.
Use require for invariants modifying the state.
- 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:
function-init-state - Severity:
Informational - Confidence:
High
Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use non-constant state variable.
contract StateVarInitFromFunction {
uint public v = set(); // Initialize from function (sets to 77)
uint public w = 5;
uint public x = set(); // Initialize from function (sets to 88)
address public shouldntBeReported = address(8);
constructor(){
// The constructor is run after all state variables are initialized.
}
function set() public returns(uint) {
// If this function is being used to initialize a state variable declared
// before w, w will be zero. If it is declared after w, w will be set.
if(w == 0) {
return 77;
}
return 88;
}
}In this case, users might intend a function to return a value a state variable can initialize with, without realizing the context for the contract is not fully initialized. In the example above, the same function sets two different values for state variables because it checks a state variable that is not yet initialized in one case, and is initialized in the other. Special care must be taken when initializing state variables from an immediate function call so as not to incorrectly assume the state is initialized.
Remove any initialization of state variables via non-constant state variables or function calls. If variables must be set upon contract deployment, locate initialization in the constructor instead.
- 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:
missing-inheritance - Severity:
Informational - Confidence:
High
Detect missing inheritance.
interface ISomething {
function f1() external returns(uint);
}
contract Something {
function f1() external returns(uint){
return 42;
}
}Something should inherit from ISomething.
Inherit from the missing interface or contract.
- 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:
redundant-statements - Severity:
Informational - Confidence:
High
Detect the usage of redundant statements that have no effect.
contract RedundantStatementsContract {
constructor() public {
uint; // Elementary Type Name
bool; // Elementary Type Name
RedundantStatementsContract; // Identifier
}
function test() public returns (uint) {
uint; // Elementary Type Name
assert; // Identifier
test; // Identifier
return 777;
}
}Each commented line references types/identifiers, but performs no action with them, so no code will be generated for such statements and they can be removed.
Remove redundant statements if they congest code but offer no value.
- 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.16 - 0.5.17
- 0.6.11 - 0.6.12
- 0.7.5 - 0.7.6
- 0.8.4 - 0.8.7 Use a simple pragma version that allows any of these versions. Consider using the latest version of Solidity for testing.
- Check:
unimplemented-functions - Severity:
Informational - Confidence:
High
Detect functions that are not implemented on derived-most contracts.
interface BaseInterface {
function f1() external returns(uint);
function f2() external returns(uint);
}
interface BaseInterface2 {
function f3() external returns(uint);
}
contract DerivedContract is BaseInterface, BaseInterface2 {
function f1() external returns(uint){
return 42;
}
}DerivedContract does not implement BaseInterface.f2 or BaseInterface2.f3.
As a result, the contract will not properly compile.
All unimplemented functions must be implemented on a contract that is meant to be used.
Implement all unimplemented functions in any contract you intend to use directly (not simply inherit from).
- Check:
unused-state - Severity:
Informational - Confidence:
High
Unused state variable.
Remove unused state variables.
- Check:
costly-loop - Severity:
Informational - Confidence:
Medium
Costly operations inside a loop might waste gas, so optimizations are justified.
contract CostlyOperationsInLoop{
uint loop_count = 100;
uint state_variable=0;
function bad() external{
for (uint i=0; i < loop_count; i++){
state_variable++;
}
}
function good() external{
uint local_variable = state_variable;
for (uint i=0; i < loop_count; i++){
local_variable++;
}
state_variable = local_variable;
}
}Incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas.
Use a local variable to hold the loop computation result.
- Check:
dead-code - Severity:
Informational - Confidence:
Medium
Functions that are not sued.
contract Contract{
function dead_code() internal() {}
}dead_code is not used in the contract, and make the code's review more difficult.
Remove unused functions.
- 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:
similar-names - Severity:
Informational - Confidence:
Medium
Detect variables with names that are too similar.
Bob uses several variables with similar names. As a result, his code is difficult to review.
Prevent variables from having similar names.
- 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.