Security Audit for PTOY Patientory Token ICO

CoinFabrik smart contract audit‘s team was hired to audit contracts written by TokenMarket for the Patientory Token ICO. The result of this security review is reflected in this document.

Audited Files

The contracts we audited are hosted at Github repository:

MintableToken.sol
CrowdsaleToken.sol
ReleasableToken.sol
UpgradeableToken.sol

Commit hash f968cffe1abf4a3d96d6705ec1befd6cfec13ae3.

Vulnerabilities Found in the Security Audit for PTOY Patientory Token ICO

Short Address Attack

The version reviewed of contract StandardToken which implements transferFrom and approve are not protected against the recently found vulnerability “ERC20 Short Address Attack”  (http://vessenes.com/the-erc20-short-address-attack-explained/).

We should note that the method transfer is protected for this attack.

They should verify the payload size that matches the size of the function parameters:

 function transferFrom(address _from, address _to, uint _value) returns (bool success) {
  uint _allowance = allowed[_from][msg.sender];

  // Check is not needed because safeSub(_allowance, _value) will already throw if this condition is not met
  // if (_value > _allowance) throw;

  balances[_to] = safeAdd(balances[_to], _value);
  balances[_from] = safeSub(balances[_from], _value);
  allowed[_from][msg.sender] = safeSub(_allowance, _value);
  Transfer(_from, _to, _value);
  return true;
 }

 function approve(address _spender, uint _value) returns (bool success) {

  // To change the approve amount you first have to reduce the addresses`
  // allowance to zero by calling `approve(_spender, 0)` if it is not
  // already 0 to mitigate the race condition described here:
  // https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
  if ((_value != 0) && (allowed[msg.sender][_spender] != 0)) throw;

  allowed[msg.sender][_spender] = _value;
  Approval(msg.sender, _spender, _value);
   return true;
 }

Should add onlyPayloadSize modifier to transferFrom and approve functions:

function transferFrom(address _from, address _to, uint _value) onlyPayloadSize(3*32) returns (bool success) {
    uint _allowance = allowed[_from][msg.sender];

    // Check is not needed because safeSub(_allowance, _value) will already throw if this condition is not met
    // if (_value > _allowance) throw;

    balances[_to] = safeAdd(balances[_to], _value);
    balances[_from] = safeSub(balances[_from], _value);
    allowed[_from][msg.sender] = safeSub(_allowance, _value);
    Transfer(_from, _to, _value);
    return true;
}
function approve(address _spender, uint _value) onlyPayloadSize(2*32) returns (bool success) {

    // To change the approve amount you first have to reduce the addresses`
    //  allowance to zero by calling `approve(_spender, 0)` if it is not
    //  already 0 to mitigate the race condition described here:
    //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
    if ((_value != 0) && (allowed[msg.sender][_spender] != 0)) throw;

    allowed[msg.sender][_spender] = _value;
    Approval(msg.sender, _spender, _value);
    return true;
}

Refund tokens

The CrowdsaleToken does not provide a method to refund received tokens. There are cases when a user by mistake has sent token to a contract address. If the contract does not provide a method to recover those tokens, those tokens are held indefinitely by the contract, and cannot be retrieved and are effectively lost.

The following implementation allows the owner to return tokens to a specified address:

Event ReclaimTokens(address indexed token, address indexed to, uint value);

function reclaimTokens(address _token, address _to) onlyOwner {
  ERC20 token = ERC20(_token);
  uint balance = token.balanceOf(this);
  if (balance > 0) {
    token.transfer(_to, balance);
    ReclaimTokens(_token, _to, balance);
  }
}

StandardToken.sol

Functions transfer, transferFrom and approve return a boolean value. They use safeAdd and safeSub which throws on error conditions. This is inconsistent with the boolean return value, they should return false on failure. Also, ReleasableToken.sol uses modifier canTransfer which throws on error.

Solidity Version

MintableToken.sol v0.4.6
CrowdsaleToken.sol v0.4.8
ReleasableToken.sol v0.4.8
UpgradeableToken.sol v0.4.8

We recommend using v0.4.11 or above since these versions are vulnerable to ConstantOptimizerSubtraction (low-severity)

Duplicated code (minor)

There are two implementations of secure mathematical operations, one in the SafeMath contract and another in the SafeMathLib library.

Having duplicated code perform the same operation it increases the cost of the deployment of the contract unnecessarily. Duplicated code might cause subtle bugs that are hard to debug and from an engineering perspective it increases the burden of testing and reviewing.

In this contract the functionality is almost identical and should not cause problems, beyond an increase in deployment costs.

Implemented improvements

Protection for double spend on approval

Tokens that implement the standard ERC20 are subject to a double spend attack when the user A approves a limit to user B by mistake, and tries to fix calling again with a new limit. If the user B retrieves the tokens between calls it will be allowed another extraction.

The contract code provides a protection against this attack. To modify an address extraction limit it first should be reset to zero. It also provides two methods addApproval and subApproval, that are not part of the ERC20 standard, that provide functionality to alter the limit without the risk of the double spend attack.

Short address attack

The methods transfer, addApproval, and subApproval are reasonably protected against the double spend attack.

  function transfer(address _to, uint _value) onlyPayloadSize(2 * 32) returns (bool success) {
  ..

  function addApproval(address _spender, uint _addedValue)
  onlyPayloadSize(2 * 32)
  returns (bool success) {
  ..

  function subApproval(address _spender, uint _subtractedValue)
  onlyPayloadSize(2 * 32)
  returns (bool success) {
  ..