General Security Coding Guide for Solidity
## I. Variable Storage/Assignment/Deletion:：
### 1.Do not store any sensitive information on the blockchain.
#### Vulnerability Details:
Due to the transparency of the blockchain, any contract data deployed on the chain is visible and transparent, even for variables that are marked as private. This is because the visibility of private variables is limited to functions and external contracts, and any user can retrieve these values by searching for them in the on-chain data. In this situation, any attempts to use private modifiers to ensure confidentiality are not secure.
There is a simple lottery code as follows:
contract Eocene{
mapping(address => bytes32) candidate;
uint private seed = 0x123413d;
function select() public{
bytes32 result = keccak256(abi.encodePacked(seed));
if(result == candidate[msg.sender]){
payable(msg.sender).transfer(1 ether);
}
}
}
// …
}In the above contract code, even if the seed has been declared as a private variable, anyone can retrieve the seed value on the chain by using the contract address and the slot position of the seed. By doing so, they can calculate the corresponding value and obtain the ETH.
#### Fix:
Do not store any critical values used for verification in the contract. Instead, store these values off-chain. Any values stored on the chain are transparent, so only implement the corresponding verification logic on the chain.
### 2.Be aware of variable default values
#### Vulnerability Details:
In Solidity, the initial value of a variable is 0/false. In this case, if the impact of the variable’s initial value is not considered when making judgments based on a certain variable, it may lead to corresponding security issues.
Consider the following airdrop unlocking code:
contract Eocene{
mapping(address => bool) unlocked;
uint averageDrop;
address token;
function setAverageDrop() public {
averageDrop = 1000;
}
function drop() public {
if(unlocked[msg.sender] == false){
ERC20(token).transfer(msg.sender,averageDrop);
}
}
}The intention of the contract code is to distribute tokens to all unlocked addresses. However, it ignores the fact that in Solidity, all variable initial values are 0/false. In a mapping type, the key is only used to concatenate with the slot and calculate the address corresponding to the key in storage using keccak256. This means that any address, whether initialized or not, will have a corresponding storage, and the initial value for this storage is usually 0/false.
#### Fix:
In any situation, do not rely on the default value of a variable to make critical judgments, especially when dealing with multiple nested variables of mapping type. Strictly prevent such issues from occurring.
### 3.Use delete to remove unused struct type values
#### Vulnerability Details:
For any mapping type, when the value field type is struct and the corresponding value is no longer needed, use delete to remove that value. Otherwise, the value will still remain in the corresponding slot.
Consider the following code:
`
contract Eocene{
struct Stake{
uint amount;
uint needReceive;
uint startTime;
}
mapping(address => Stake) stakes;
mapping(address => bool) staker;
function getStake() public{
Stake memory _stake = stakes[msg.sender];
(msg.sender).transfer(_stake.needReceive);
staker[msg.sender] = false;
// delete stakes[msg.sender] // need do but don't
}
function calReceive() public{
require(staker[msg.sender],'not staker');
stakes[msg.sender].needReceive = stakes[msg.sender].amount * (block.time - stakes[msg.sender].startTime);
stakes[msg.sender].amount = 0;
}
}In the above contract code, ETH is calculated based on the amount and duration of the stake. However, after the stake is completed, only the value of staker[msg.sender] is set to false, while the corresponding stakes[msg.sender] still exists. Therefore, an attacker can call the getStake() function indefinitely to obtain ETH.
#### Fix:
Of course, the above code also has some other auxiliary issues that lead to vulnerabilities. However, you should be aware that for struct type variables stored in storage, the entire struct value should be deleted using delete (or all values should be set to 0) when no longer needed, otherwise the value will continue to exist in the corresponding slot.
## II. Function Definition:
### 1.Explicitly Declare Function Visibility
#### Vulnerability Details:
The default visibility of a function is public. For any function, its visibility must be explicitly declared to prevent vulnerabilities caused by negligence, especially when calling underlying functions with nested layers, to prevent the underlying functions from being incorrectly assigned visibility due to negligence.
Consider the following vulnerable code example:
contract Eocene{
mapping(address => bool) whitelist;
function _a() {
payable(msg.sender).transfer(1 ether);
}
function a() public{
require(whitelist[msg.sender],'not in whitelist');
_a();
}
}The function a() restricts whitelist addresses through require and transfers funds to the corresponding address. Normally, _a() should not be called externally, but here it is mistakenly declared as public due to the lack of explicit visibility declaration, making it directly callable externally.
#### Fix:
Declare the visibility of all functions explicitly, especially for functions that cannot be called directly from outside, which must be explicitly declared as protected or private.
### 2.Reentrancy Attack
#### Vulnerability Details:
For any function, consider the issues that may arise after reentrancy. Reentrancy here includes all reentrancy problems caused by external calls such as transfer/send/call/staticcall.
Consider the following code:
contract Fund {
/// @dev Mapping of ether shares of the contract.
mapping(address => uint) shares;
/// Withdraw your share.
function withdraw() public {
if (payable(msg.sender).send(shares[msg.sender]))
shares[msg.sender] = 0;
}
}For the above contract, when msg.sender is malicious, it can lead to msg.sender withdrawing all the current contract balance without limitation. However, we must also be aware that calling transfer/send/call/staticcall and any external contract functions may cause reentrancy problems.
#### Fix:
Depending on the specific implementation of the contract, modify the key variable implementation first. For example, in the above contract, you can first record the value of shares[msg.sender], set shares[msg.sender] to 0, and then perform the send operation. Of course, this can also be achieved through a combination of global variables and modifiers.
III. External Interaction
### 1. Restrict List of Addresses and Function Names for External Calls
#### Vulnerability Details:
When calling an external function, the contract address and function name must be limited under reasonable circumstances. This is because you cannot determine whether the contract function of any external address will change. Therefore, in possible situations, the contract address and function name for external calls must be strictly limited to prevent security issues in the contract caused by the insecurity of external contracts as much as possible.
Consider the following code:
contract Eocene{
function callExt(address _target,bytes calldata data) public{
_target.call(data);
}
function delegateCallExt(address _target,bytes calldata data) public{
_target.delegatecall(data);
}
}The function callExt() is used to call any function at any address. This can easily lead to reentrancy problems and, once the contract has any token assets in any wallet, the corresponding token’s transfer function can be called directly through this function to transfer the assets away.
If there are no restrictions when calling the delegateCallExt() function, it may cause the contract to be destructed directly, causing the entire contract address balance to be transferred away and the contract to be destroyed.
#### Fix:
When calling any external contract, consider whether the address can be whitelisted first, and then further consider whether the function name of the specified address can be restricted.
### 2.When using call, send, delegatecall, staticcall, do not rely solely on exceptions to judge external calls, but also use return values to determine if the execution is successful.
#### Vulnerability Details:
The above function will not revert due to internal errors, but only returns a revert message. Whenever using them, you must judge the success of the execution by the return value of the function.
Consider the following code example:
contract Eocene{
address token; //any token address
function deposit(uint amount) public{
token.call(abi.EncodeWithSignature("transferfrom(address from,address receipt,uint amount)"),msg.sender,address(this),amount);
mint(msg.sender,amount);
}
}The deposit() function first tries to transfer the specified token from msg.sender to the current address. If the transfer is successful, it mints some of the current currency for msg.sender. But because the call() function does not revert the entire transaction when it fails, even if no currency is transferred from msg.sender to the current address, msg.sender will still be minted amount of the current currency.
#### Fix:
The judgment of the execution result of call/send/delegatecall/staticcall must be based on their return values, not on whether they revert or not.
## IV. Access Control:
### 1.Do Not Base Identity Verification on tx.origin
#### Vulnerability Details:
Do not base identity verification on tx.origin, which is the initiator of the entire transaction and does not change with recursive calls of the contract. Any authentication based on tx.origin cannot guarantee that tx.origin is msg.sender, and such authentication also increases the security risk of users’ accounts.
Consider the following vulnerable example:
contract Eocene{
mapping(address=>bool) whitelist;
function freeDeposit() public{
require(whitelist[tx.origin],'not in whitelist');
payable(msg.sender).transfer(1 ether);
}
}When any address in the whitelist is induced by some phishing link to call any seemingly harmless malicious contract address and function, and the malicious address calls the freeDeposit function of the example code, the assets that should belong to the whitelisted address will be transferred to the malicious contract address.
#### Fix:
Do not base identity verification on tx.origin. For the above code, changing to payable(tx.origin).transfer(1 ether) will not cause problems. However, the recommended approach is not to use tx.origin for identity verification, but to use require(whitelist[msg.sender],’not in whitelist’) for judgment.
### 2.Do Not Base Judgments on extcodesize Return Value for EOS Accounts
#### Vulnerability Details:
During the initialization phase of contract code, even if the address is a contract address, the return value of extcodesize will be 0. If judgments are based on this return value, the result will be inaccurate.
Consider the following code:
contract Eocene{
function withdraw() public{
uint size;
assembly {
size := extcodesize(caller())
}
require(size==0,"not eos account");
msg.sender.transfer(1 ether);
}
}In the withdraw function, the appeal contract hopes to restrict token access only to EOS accounts by limiting it through the extcodesize return value. However, it ignores the fact that during the contract initialization phase, the extcodesize return value for the contract address is also 0. This leads to an inaccurate judgment, allowing any address to obtain tokens from the contract.
#### Fix:
Do not judge based on whether an external address will be a contract address at any time, and try to ensure that the contract code functions properly under any type of account.
## V. Arithmetic Operations
### 1. Consider Overflow When Doing Any Numeric Operation
#### Vulnerability Details:
Overflow problem refers to the overflow problem caused by integer operation in the contract. The main reason is that any numeric type has its maximum length, and when the operation of two integers exceeds its maximum value, the overflow part will be truncated, causing problems.
Consider the following code format:
contract Eocene{
mapping(address=>uint) balanceof;
function withdraw(uint amount) public{
payable(msg.sender).transfer(amount);
balanceof[msg.sender] = balanceof[msg.sender]-amount;
require(balanceof[msg.sender] >= 0,'not enough balance');
}
}For the above function, consider when balanceof[msg.sender] < amount, because the type of balanceof is defined as an unsigned integer, the final calculation result will cause a negative value of the int type. When converted to the uint type, it becomes a very large positive value. At this time, the restriction condition of require is bypassed, and the attacker can steal any number of tokens from the contract.
#### Fix:
Use the SafeMath library or check the correctness of the value before each calculation to ensure that the final calculation result will not cause overflow.
### 2.Be Cautious About Using int Type When Doing Any Integer Operation
#### Vulnerability Details:
When doing any integer operation, be careful not to convert uint types into int types for calculations, unless you need this operation. Because when converting a uint type integer into an int type, some situations where the uint type is overflowed will be invalid in the int type.
Consider the following code format:
contract Eocene{
int public result;
uint public uresult;
function cal(uint _a, uint _b) public{
result = int(_a)-int(_b);
uresult = uint(result);
}
}When compiling with Solidity version 0.8.0 or above, if you call cal(0,1), even if 0–1 causes overflow in the uint, it will not cause revert due to overflow in the int calculation (because the result of 0–1 is within the range of the int type). However, when the result value is converted back to the uint type, it is the result value obtained by actually overflowing the uint type calculation, which indirectly causes the existence of overflow problems.
However, it should be noted that calling cal(type(int). Min, type(int). Max) here will still cause a revert because integer calculation exceeds the range of the int type.
#### Fix:
Be cautious about using the int type when doing any form of integer operation. If the integer operation itself needs overflow, consider using uncheck to wrap the uint type operation to achieve it.
### 3.In Any Operation That May Lose Precision, Extend the Precision Through Extension
#### Vulnerability Details:
When doing any integer operation, consider the possibility of precision loss caused by overflow and extend its precision.
Consider the following code format:
contract Eocene {
uint totalsupply;
mapping(address=>uint) balancesof;
uint BasePrice = 1e16;
function mint() public payable {
uint tokens = msg.value/BasePrice;
balancesof[msg.sender] += tokens;
totalsupply += tokens;
}
}Consider the above contract. In the mint function, calculate the number of tokens that should be obtained through msg.value / basePrice. However, due to the precision problem of / calculation, the part of msg.value less than 1e16 will be all locked in the contract, which will not only waste eth, but also be very bad for users’ experience.
#### Fix:
In any integer calculation that may have a precision loss, first use *1eN to extend the integer (N is the required precision size).
## Ⅵ、Randomness：
### 1.Do not use any predictable/manipulatable on-chain data as random number seeds.
#### Vulnerability Details:
Due to the unique nature of blockchain, there are no truly random values on-chain. Therefore, no on-chain data should be used as random values or random number seeds. It is better to obtain random values off-chain instead.
The following code example demonstrates this vulnerability:
contract Eocene{
function winner(bytes32 value) public payable{
require(msg.value > 0.5 ether,"not enough value");
if(value == keccak256(abi.encodePacked(block.timestamp))){
msg.sender.transfer(1 ether);
}
}
}For this contract, a random value is calculated based on the current block timestamp and compared with the value submitted by the user. If the two values match, the user receives a reward. At first glance, it seems to be a time-based random situation, but in reality, any user who uses keccak256(abi.encodePacked(block.timestamp)) can compute the same value through a contract call, and then send it to the winner() function of the contract to obtain ETH. Furthermore, we should be aware that block.timestamp is a value that can be maliciously tampered with by miners and is not necessarily fair.
#### Fix:
Do not use any on-chain data (block.*/now) as a random number or random number seed. Consider obtaining offline random values through Chainlink or other similar sources.
## Ⅶ、DOS：
### 1.Prohibit any operation that copies the entire dynamic array to a memory variable.
#### Vulnerability Details:
Solidity’s use of available memory size is much smaller than storage(0xffffffffffffffff). Any behavior that copies a dynamic array as a whole into memory may exceed the available memory size and cause a revert.
Consider the following code:
contract Eocene{
uint[] id;
function pop(uint amount) public{
require(amount>0,'not valid amount');
uint[] memory _id=id; // this may be revert because of memory space limit
for(uint i=0;i<_id.length;i++)
{
if(amount==_id[i]){
id[i] = 0;
}
}
}
function push(uint amount) public{
require(amount>0,'not valid amount');
id.push(amount);
}
}In this code, `uint[] memory _id=id;` will put the value of the variable `uint[] id;` in storage into memory. The push() function can insert values into uint[] id;. However, due to Solidity’s limitations on memory space, once the length of `uint[] id;` exceeds `(0xffffffffffffffff-0x40)/0x20–1`, it will cause excessive memory usage, resulting in a revert. This means that the pop() function of the contract cannot be executed successfully, or in other words, any function with the operation `uint[] memory _id=id;` will not be able to execute successfully.
#### Fix:
Never perform an operation where a mutable dynamic array is copied to memory. In addition, note that 0xffffffffffffffff is the maximum available memory size within Solidity functions, and any function with a memory usage that exceeds this value will not execute successfully.
### 2.Loop condition in any for loop cannot be based on externally modifiable variables
#### Vulnerability Details:
f the condition of any for loop is based on an externally modifiable variable, there may be a problem of excessive gas consumption due to the external variable being too large. When the gas consumption is high enough to exceed the tolerance of each contract caller, a DOS attack may occur.
Consider the following code:
contract Eocene{
uint[] id;
function pop(uint amount) public{
require(amount>0,'not valid amount');
for(uint i=0;i<id.length;i++)
{
if(amount==id[i]){
id[i] = 0;
}
}
}
function push(uint amount) public{
require(amount>0,'not valid amount');
id.push(amount);
}
}Here, we have removed the operation of copying the array from storage to memory, but another issue with this code is that the for loop is based on the length of `uint[] id;`, and the length of id can only increase in the contract and not decrease. This means that the gas consumed by the pop() function will continue to increase, and when the gas consumption becomes greater than the maximum gas consumption that the pop() function can tolerate, few people will execute pop(), effectively achieving a DOS attack.
#### Fix:
Prevent unbounded loops based on externally modifiable variables. Any loop operation should be able to determine the maximum length it can execute, to prevent DOS attacks from occurring.
### 3.Use try/catch to capture unknown exceptions in loops
#### Vulnerability Details:
If there is a possibility of a revert due to an external address in any internal loop, consideration must be given to catching the revert. Otherwise, once an internal loop execution fails, all previous gas consumption becomes meaningless. If the success or failure of an internal loop can be controlled by an external address, failure to catch possible exceptions with try/catch may cause the loop condition to never complete, resulting in a DOS attack.
The example code is as follows:
contract Eocene{
address[] candidates;
mapping(address=>uint) balanceof;
function claim() public{
for(uint i=0;i<candidates.length;i++)
{
address candidate = candidates[i];
require(balanceof[candidate]>0,'no balance');
payable(candidate).transfer(balanceof[candidate]);
}
}
}The code uses a for loop to transfer funds to each candidate, but it does not consider the situation where any candidate directly reverts in the fallback or receive function, causing the loop to never execute successfully and implementing a DOS attack.
#### Fix:
In any for loop that involves an external call, and if it is impossible to determine whether the call will revert, try/catch must be used to handle exceptions and prevent DOS attacks caused by reverts.
## VIII. Use a higher version compiler:
### 1.Compile contracts using a compiler version 0.8.17 or higher.
Contracts below version 0.8.17 have some medium-high risk vulnerability issues that may expose contract code to danger. These issues exist in the compilation phase and are not easy to detect, especially when there are insufficient test cases. It is recommended to use a higher version of the compiler to avoid these problems. Of course, if you must use an affected compiler version, please make sure you understand its risks and seek professional help from security experts.
For more information on specific compiler vulnerabilities and their harm, refer to:：
- [SOL-2022–7]
- [SOL-2022–6]
- [Solidity]
## About us
At Eocene Research, we provide the insights of intentions and security behind everything you know or don’t know of blockchain, and empower every individual and organization to answer complex questions we hadn’t even dreamed of back then.
