Smart contracts are decentralized programs with code and storage publicly available. Each operation can be reviewed by a third party and the outcome for given inputs can be predicted. With decentralized applications, the main source of vulnerabilities is code logic.
Programming practices have an easy rule for a safe code - keep it clean and simple. If an outcome of each instruction is easy to determine it is easy to find when the behavior is incorrect.
Unit tests are essential for logic verification. Many problems can be found, if the code is fully covered with tests, including checks for all inputs, conditions, and most importantly negative scenarios.
Common vulnerabilities
There are a lot of simple vulnerabilities that can be found by reading code line-by-line.
Let’s explore some vulnerabilities by writing a Dummy Token contract example.
pragma solidity >=0.8.0;
contract DummyToken {
uint256 public price;
mapping(address => uint256) public balances;
constructor(uint256 _price) {
price = _price;
}
function buy() external payable {
require(msg.value == price, "INVALID_VALUE");
balances[msg.sender] += 1;
}
}There is not much functionality in it yet. When the contract is deployed the price would be set and any user would be available to buy a token if sufficient ETH is provided.
The price heavily depends on the value used during deployment. We could not change the price, so let’s fix this by adding a new function:
function setPrice(uint256 _price) external {
price = _price;
}Now we can update the price at any time, but this function introduced a few problems:
Missing sender validation - anyone can change the price
Missing inputs validation - if the price is set to 0 then tokens could be bought for free
To validate the sender we need to store the address that is allowed to perform admin operations.
address owner;
constructor(uint256 _price) {
require(_price > 0, "INVALID_PRICE");
price = _price;
owner = msg.sender;
}
modifier onlyOwner() {
require(msg.sender == owner, "NOT_OWNER");
_;
}
function setPrice(uint256 _price) external onlyOwner {
require(_price > 0, "INVALID_PRICE");
price = _price;
}
Now the owner would be set to the address that deployed the contract, and only this address would be able to modify the price. Also, we added the check that the price is greater than 0 in both the constructor and setter function. It is recommended to do a sanity check of important values in the constructor too.
The next logic that we can add to this contract is fees - every time the user buys a token, 5% of the revenue should go to a dedicated address.
function processFee(uint256 amount) private {
uint256 fee = (amount / 100) * 5; // 5%
(bool success, ) = feeCollector.call{value: fee}("");
require(success, "SEND_FEE_ERROR");
}
function setFeeCollector(address _feeCollector) external onlyOwner {
feeCollector = _feeCollector;
}
The math may seem to be correct - we calculate how much is 1% and then multiply it to get 5%. Unfortunately, the division of integers returns only a quotient and a reminder would be lost.
If in our example the amount is 40, then (40/100)*5 will return 0, even so, the correct result is 2. You should always perform multiplication before division.
Also, we added a function to set feeCollector address, but the constructor remains unchanged. Uninitialized variables by default set to 0. It means that by default feeCollector would be address(0), and due to missing validation, we can manually set this value. ETH sent to address(0) would be lost.
Now users can buy tokens but could not use them. Let’s give them the ability to transfer a token to somebody.
function transfer(address receiver) external {
balances[msg.sender] -= 1;
balances[receiver] += 1;
}
The function is very simple - we just decrease the balance of a sender and add the token to receiver address. You might notice that there are no checks on whether the sender has tokens on their balance. In this example compiler version is set to >= 0.8.0, where solidity automatically checks for underflow/overflow case. If we try to subtract from a zero, the error will be generated. Previous to solidity 0.8.0 it was required to manually check for underflow/overflow or to use a special library like SafeMath.
Let’s also add a function that can transfer tokens to 2 receivers.
function transfer(address receiver) external {
transferFrom(msg.sender, receiver);
}
function transferMany(address receiver1, address receiver2)
external
{
transferFrom(msg.sender, receiver1);
transferFrom(msg.sender, receiver2);
}
function transferFrom(address from, address receiver) {
balances[from] -= 1;
balances[receiver] += 1;
}
To prevent code duplication we added a helper function transferFrom that should be used by transfer and transferMany. Because we “forgot” to specify the visibility transferFrom, the default visibility of the function is set to public. This will allow transferring tokens from any address because it is accepted as a parameter. Public functions should be evaluated whether they need to be accessible from the outside.
Now it’s time to apply all fixes:
pragma solidity >=0.8.0;
contract DummyToken {
uint256 public price;
mapping(address => uint256) public balances;
address public owner;
address public feeCollector;
constructor(uint256 _price, address _feeCollector) {
owner = msg.sender;
setPrice(_price);
setFeeCollector(_feeCollector);
}
modifier onlyOwner() {
require(msg.sender == owner, "NOT_OWNER");
_;
}
function buy() external payable {
require(msg.value == price, "INVALID_VALUE");
balances[msg.sender] += 1;
}
function setPrice(uint256 _price) public onlyOwner {
require(_price > 0, "INVALID_PRICE");
price = _price;
}
function processFee(uint256 amount) private {
uint256 fee = amount / 20; // 5%
(bool success, ) = feeCollector.call{value: fee}("");
require(success, "SEND_FEE_ERROR");
}
function setFeeCollector(address _feeCollector) public onlyOwner {
require(_feeCollector != address(0), "INVALID_FEE_ADDRESS");
feeCollector = _feeCollector;
}
function transfer(address receiver) external {
transferFrom(msg.sender, receiver);
}
function transferMany(address receiver1, address receiver2)
external
{
transferFrom(msg.sender, receiver1);
transferFrom(msg.sender, receiver2);
}
function transferFrom(address from, address receiver) internal {
balances[from] -= 1;
balances[receiver] += 1;
}
}
The contract looks to be completed, but there still one important part missing. When the token is bought, 95% of the price will remain on the contract balance, but there is no way to withdraw it. It is important to ensure that the contract has a withdraw ETH functionality if it has payable functions.
function withdrawEth() external {
(bool success, ) = owner.call{value: address(this).balance}("");
require(success, "WITHDRAW_ERROR");
}
Problems with inheritance
Some of the problems are not so obvious. Let’s look at a few examples of contracts that are using inheritance.
contract Base1 {
function getValue() public returns(uint) {
return 1;
}
}
contract Base2 {
function getValue() public returns(uint) {
return 2;
}
}
contract Derived is Base1, Base2 {
}
Solidity supports multiple inheritance. What will be the result when calling getValue() from the Derived contract? Solidity will use the implementation from the last inherited contract. When working with multiple inheritance specify contracts in the correct order.
A situation when a Base and Derived contracts have variables with the same name is called variable shadowing.
contract Base {
uint private x = 1;
function getValueBase() public returns(uint) {
return x;
}
}
contract Derived is Base {
uint private x = 2;
function getValue() public returns(uint) {
return x;
}
}
When variable shadowing occurs, both variables would be created in storage and they would be accessible from the corresponding contract. In the example contract Derived::getValueBase() will return 1.
Some problems may occur when overriding a function, especially if the base contract is implemented by someone else (for example by openzeppelin).
contract Base {
function pay() external payable {
afterPayment();
}
function afterPayment() internal virtual {
// do nothing
}
}
contract Derived is Base {
uint public payments;
function afterPayment() internal override {
++payments;
}
function getAvaragePayment() external returns(uint) {
return address(this).balance / payments;
}
}
When overriding a function that is called internally in the Base contract it is important to check how this function is used. In the example contract, we override afterPayment() function to letter check the average amount of payment, but the Base contract has no checks for zero-payments.
Conclusion
In this post we explored how to determine some issues:
Check if a value should be changed after deployment;
Check sender validation;
Check inputs validation;
Perform multiplication before division;
Remember that uninitialized variables are 0 by default;
Check that ETH is not sent to address(0);
Check that underflow/overflow is handled;
Check that every function specifies visibility level;
Validate that public function should be accessible from the outside;
Ensure that the contract has a withdraw ETH functionality if it has payable functions;
Check inheritance order;
Check variable shadowing;
Check how overridden functions are used in the base contract.