sunbeomso commented 4 years ago

Description

Could you check the Mythril's result below, which involves array inputs?

Before all, I used the following abi-decoders after I properly padded every input data as commented in https://github.com/ConsenSys/mythril/issues/1319#issuecomment-581139936:

I have the following code containing a potential integer overflow at line 2:

     function batchTransfer(address[] _receivers, uint256 _value) public whenNotPaused returns (bool) 
   {                                                                                                                                                    
1.    uint cnt = _receivers.length;                                                                       
2.    uint256 amount = uint256(cnt) * _value;    /// IO in multiplication here
3.    require(cnt > 0 && cnt <= 20);                                                                      
       require(_value > 0 && balances[msg.sender] >= amount);                                              
       balances[msg.sender] = balances[msg.sender].sub(amount);                                            
       for (uint i = 0; i < cnt; i++) {                                                                    
           balances[_receivers[i]] = balances[_receivers[i]].add(_value);                                  
           Transfer(msg.sender, _receivers[i], _value);                                                    
       }                                                                                                   
       return true;                                                                                        
     }                                                                                                     
   }

Mythril generated the following two transaction sequences to detect the overflow at line2 :

0x83f12fec83ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffde00000000000000000000000000000000000000000f
0x83f12fec8040000000000000000000000000000000000000000000000000000000000004555555555555555555555555555555555555555556 , each of which decoded into the following transaction, respectively:
- batchTransfer([], 4642275147320176030871715840)
- batchTransfer([], 38597363079105398474523661669562635951089994888547061003159075566016859734016)

However, the first transaction is reverted due to require(cnt > 0 && cnt <= 20); at line 3, since its first argument is an empty array, which seems strange.

How to Reproduce

Address of the contract: https://etherscan.io/address/0xc5d105e63711398af9bbff092d4b6769c82f793d#code

My command:

sudo docker exec CONTAINER myth analyze /tmp/test.sol:BecToken --solv 0.4.25 -m integer,exceptions

Expected behavior

Mythril should not generate an empty array as an input in this contract.

Environment

Mythril version: v 0.21.21 (from docker image)
Solidity compiler and version: 0.5.12
Python version: 3.6.9 (the one installed in the docker container)
OS and Version: Ubuntu 18.04 (Host OS)

sunbeomso commented 4 years ago

You can find the function at line 259--271.

norhh commented 4 years ago

When the code is moved to 0.5.0


/**
 * @title SafeMath
 * @dev Math operations with safety checks that throw on error
 */
library SafeMath {
  function mul(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a * b;
    assert(a == 0 || c / a == b);
    return c;
  }

  function div(uint256 a, uint256 b) internal pure returns (uint256) {
    // assert(b > 0); // Solidity automatically throws when dividing by 0
    uint256 c = a / b;
    // assert(a == b * c + a % b); // There is no case in which this doesn't hold
    return c;
  }

  function sub(uint256 a, uint256 b) internal pure returns (uint256) {
    assert(b <= a);
    return a - b;
  }

  function add(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a + b;
    assert(c >= a);
    return c;
  }
}

/**
 * @title ERC20Basic
 * @dev Simpler version of ERC20 interface
 * @dev see https://github.com/ethereum/EIPs/issues/179
 */
contract ERC20Basic {
  uint256 public totalSupply;
  function balanceOf(address who) public returns (uint256);
  function transfer(address to, uint256 value) public returns (bool);
  event Transfer(address indexed from, address indexed to, uint256 value);
}

/**
 * @title Basic token
 * @dev Basic version of StandardToken, with no allowances.
 */
contract BasicToken is ERC20Basic {
  using SafeMath for uint256;

  mapping(address => uint256) balances;

  /**
  * @dev transfer token for a specified address
  * @param _to The address to transfer to.
  * @param _value The amount to be transferred.
  */
  function transfer(address _to, uint256 _value) public returns (bool) {
    require(_to != address(0));
    require(_value > 0 && _value <= balances[msg.sender]);

    // SafeMath.sub will throw if there is not enough balance.
    balances[msg.sender] = balances[msg.sender].sub(_value);
    balances[_to] = balances[_to].add(_value);
    emit Transfer(msg.sender, _to, _value);
    return true;
  }

  /**
  * @dev Gets the balance of the specified address.
  * @param _owner The address to query the the balance of.
  * @return An uint256 representing the amount owned by the passed address.
  */
  function balanceOf(address _owner) public returns (uint256 balance) {
    return balances[_owner];
  }
}

/**
 * @title ERC20 interface
 * @dev see https://github.com/ethereum/EIPs/issues/20
 */
contract ERC20 is ERC20Basic {
  function allowance(address owner, address spender) public returns (uint256);
  function transferFrom(address from, address to, uint256 value) public returns (bool);
  function approve(address spender, uint256 value) public returns (bool);
  event Approval(address indexed owner, address indexed spender, uint256 value);
}

/**
 * @title Standard ERC20 token
 *
 * @dev Implementation of the basic standard token.
 * @dev https://github.com/ethereum/EIPs/issues/20
 * @dev Based on code by FirstBlood: https://github.com/Firstbloodio/token/blob/master/smart_contract/FirstBloodToken.sol
 */
contract StandardToken is ERC20, BasicToken {

  mapping (address => mapping (address => uint256)) internal allowed;

  /**
   * @dev Transfer tokens from one address to another
   * @param _from address The address which you want to send tokens from
   * @param _to address The address which you want to transfer to
   * @param _value uint256 the amount of tokens to be transferred
   */
  function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
    require(_to != address(0));
    require(_value > 0 && _value <= balances[_from]);
    require(_value <= allowed[_from][msg.sender]);

    balances[_from] = balances[_from].sub(_value);
    balances[_to] = balances[_to].add(_value);
    allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
    emit Transfer(_from, _to, _value);
    return true;
  }

  /**
   * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
   *
   * Beware that changing an allowance with this method brings the risk that someone may use both the old
   * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
   * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
   * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
   * @param _spender The address which will spend the funds.
   * @param _value The amount of tokens to be spent.
   */
  function approve(address _spender, uint256 _value) public returns (bool) {
    allowed[msg.sender][_spender] = _value;
    emit Approval(msg.sender, _spender, _value);
    return true;
  }

  /**
   * @dev Function to check the amount of tokens that an owner allowed to a spender.
   * @param _owner address The address which owns the funds.
   * @param _spender address The address which will spend the funds.
   * @return A uint256 specifying the amount of tokens still available for the spender.
   */
  function allowance(address _owner, address _spender) public returns (uint256 remaining) {
    return allowed[_owner][_spender];
  }
}

/**
 * @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 public 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() public {
    owner = msg.sender;
  }

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

  /**
   * @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) onlyOwner public {
    require(newOwner != address(0));
    emit OwnershipTransferred(owner, newOwner);
    owner = newOwner;
  }

}

/**
 * @title Pausable
 * @dev Base contract which allows children to implement an emergency stop mechanism.
 */
contract Pausable is Ownable {
  event Pause();
  event Unpause();

  bool public paused = false;

  /**
   * @dev Modifier to make a function callable only when the contract is not paused.
   */
  modifier whenNotPaused() {
    require(!paused);
    _;
  }

  /**
   * @dev Modifier to make a function callable only when the contract is paused.
   */
  modifier whenPaused() {
    require(paused);
    _;
  }

  /**
   * @dev called by the owner to pause, triggers stopped state
   */
  function pause() onlyOwner whenNotPaused public {
    paused = true;
    emit Pause();
  }

  /**
   * @dev called by the owner to unpause, returns to normal state
   */
  function unpause() onlyOwner whenPaused public {
    paused = false;
    emit Unpause();
  }
}

/**
 * @title Pausable token
 *
 * @dev StandardToken modified with pausable transfers.
 **/

contract PausableToken is StandardToken, Pausable {

  function transfer(address _to, uint256 _value) public whenNotPaused returns (bool) {
    return super.transfer(_to, _value);
  }

  function transferFrom(address _from, address _to, uint256 _value) public whenNotPaused returns (bool) {
    return super.transferFrom(_from, _to, _value);
  }

  function approve(address _spender, uint256 _value) public whenNotPaused returns (bool) {
    return super.approve(_spender, _value);
  }

  function batchTransfer(address[] memory _receivers, uint256 _value) public whenNotPaused returns (bool) {
    uint cnt = _receivers.length;
    uint256 amount = uint256(cnt) * _value;
    require(cnt > 0 && cnt <= 20);
    require(_value > 0 && balances[msg.sender] >= amount);

    balances[msg.sender] = balances[msg.sender].sub(amount);
    for (uint i = 0; i < cnt; i++) {
        balances[_receivers[i]] = balances[_receivers[i]].add(_value);
        emit Transfer(msg.sender, _receivers[i], _value);
    }
    return true;
  }
}

/**
 * @title Bec Token
 *
 * @dev Implementation of Bec Token based on the basic standard token.
 */
contract BecToken is PausableToken {
    /**
    * Public variables of the token
    * The following variables are OPTIONAL vanities. One does not have to include them.
    * They allow one to customise the token contract & in no way influences the core functionality.
    * Some wallets/interfaces might not even bother to look at this information.
    */
    string public name = "BeautyChain";
    string public symbol = "BEC";
    string public version = '1.0.0';
    uint8 public decimals = 18;

    /**
     * @dev Function to check the amount of tokens that an owner allowed to a spender.
     */
    constructor() public {
      totalSupply = 7000000000 * (10**(uint256(decimals)));
      balances[msg.sender] = totalSupply;    // Give the creator all initial tokens
    }

    function () external {
        //if ether is sent to this address, send it back.
        revert();
    }
}

I ran this code on both 0.5.0 and 0.4.25. In the case of 0.5.0, it shows a proper output, whereas in the case of 0.4.25 it is showing this output, which isn't right.

sunbeomso commented 4 years ago

Thanks for letting me know. But the situation is unclear for me.

norhh commented 4 years ago

The older solidity versions doesn't implement checks for calldata. Due to which the source code and bytecode aren't exactly equivalent. For the first example the decoded input is batchTransfer([], 4642275147320176030871715840) The expected multiplication is 04642275147320176030871715840. But what happens is something totally different as solidity assumes the `calldata >= 4 + 64no_of_argswhich mythril doesn't assume as it's a bytecode execution engine(it can't assume as bytecode doesn't have anything called an argument). the offset to calldataoffset=83ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffis a negative number so the behavior is undefined and it gets some weird calldata from somewhere as additions and inequality checks break in many places, so the array isn't[]` for that input according to older solidity versions.

Consensys / mythril

Decoding array inputs from Mythril #1320

Description

How to Reproduce

Expected behavior

Environment