Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit updated Approval in transferFrom #707

Closed
frangio opened this issue Jan 25, 2018 · 7 comments
Closed

Emit updated Approval in transferFrom #707

frangio opened this issue Jan 25, 2018 · 7 comments
Assignees
@nventuro
Labels
contracts Smart contract code.
Milestone
v2.1

Comments

@frangio
Copy link
Contributor

frangio commented Jan 25, 2018

@martriay has noticed that Approval events are not sufficient to reconstruct the allowance status of an address, because allowance is silently modified (reduced) in transferFrom. In fact, even all of the ERC20 events together don't suffice either, because of the long-standing problem that transferFrom transfers are logged like simple Transfer events with no information on who the spender was.

I want us to evaluate the possibility of emitting an Approval event in the transferFrom function to signal the reduced allowance, if it is ERC20 compliant. I also wonder if this can solve the problem of transferFrom being a "silent" operation, by emitting a recognizable sequence of Transfer and Approval events, but it shouldn't allow the token holder to simulate a transferFrom.
@shrugs
Copy link
Contributor

shrugs commented Feb 20, 2018

I'd agree that this would be valuable—in the future I expect quite a few usecases will pop up where people would like to watch for events in order to update off-chain state. If it's not possible to recover the full state using the events, long-polling the contract becomes necessary, which isn't really ideal.

@frangio frangio self-assigned this Apr 9, 2018
@frangio frangio mentioned this issue Apr 9, 2018
Merged
@frangio frangio added the contracts Smart contract code. label Aug 12, 2018
@frangio frangio added this to the v2.0 milestone Aug 12, 2018
@nventuro
Copy link
Contributor

From my interpretation of the spec, this would be compliant, since it doesn't specify that Approval must only be emitted on approve.

Additionally, a transferFrom call followed by an approve call, setting the allowance to the current allowance value, would emit the exact same two events while not modifying the state in any way, making this a completely valid and useful change IMO.

@frangio
Copy link
Contributor Author

frangio commented Aug 12, 2018

Here's an argument against, to make for a balanced discussion.

The ERC20 implementations around probably all have the problem that events are not enough to reconstruct allowance. Because of this, all applications will have to work around this by either polling or some other complex mechanism. What value would we be adding, then?

If an application is written specifically to interact with an OpenZeppelin ERC20 instance, and thus can safely rely on events only, it may silently become incorrect when plugged into a non-OpenZeppelin instance. I suppose we could try to consider the implications and base our decision on that, but it feels risky.

@shrugs
Copy link
Contributor

shrugs commented Aug 12, 2018

hmm good points. this problems exists across a lot of domains, though; when the world moves forward, you have to decide which world you support; the new, the old, or both.

In the case of early ERCs, it's impossible to know which world you're in because we don't have migrations or 165 interfaces to depend on, so it gets really hazy.

I'm in favor of adding the event here and telling people that it's a reverse-breaking change (is there a better word for that?)

But we should also consider adding 165 interfaces to erc20 as well, for similar reasons.

@nventuro
Copy link
Contributor

I agree with @shrugs, we should push the industry forward, not stagnate because the standard isn't as good as it could be. Good design doesn't happen in a vacuum, these kind of improvements may drive new standards.

More pragmatically, an application may not need to interact with multiple tokens, in which case using our enhanced one may make their life easier.

ardagokcek added a commit to ardagokcek/openzeppelin-solidity that referenced this issue Sep 2, 2018
@ardagokcek
Adding TransferFrom Event
1448915 
Check Issue OpenZeppelin#707 these changes makes the idea with adding the event in IERC20.sol 
Event: 
event TransferFrom(
      address indexed executor,
      address indexed from,
      address indexed to,
      uint256 value);
@ardagokcek ardagokcek mentioned this issue Sep 2, 2018
Closed
4 tasks
@frangio frangio modified the milestones: v2.0, v2.1 Sep 4, 2018
@frangio frangio modified the milestones: Test helpers, v2.1 Nov 20, 2018
@Dycedlp Dycedlp mentioned this issue Nov 24, 2018
Closed
@frangio frangio changed the title Evaluate possibility of emitting Approval in transferFrom Emit updated Approval in transferFrom Nov 27, 2018
@frangio frangio assigned nventuro and unassigned frangio Nov 27, 2018
@nventuro
Copy link
Contributor

I think the arguments exposed are good enough to warrant this being a thing, will open a PR soon.

@nventuro nventuro mentioned this issue Nov 28, 2018
Merged
@truls truls mentioned this issue Jan 26, 2019
Merged
@nventuro nventuro mentioned this issue Oct 18, 2019
Closed
@k06a k06a mentioned this issue Oct 18, 2019
Open
8 tasks
aburaza added a commit to aburaza/payusd that referenced this issue Nov 21, 2019
@aburaza
payusd.sol
fbdfe31 
pragma solidity ^0.4.24;



/**
 * @title ERC20 interface
 * @dev see ethereum/EIPs#20
 */
interface IERC20 {
  function totalSupply() external view returns (uint256);

  function balanceOf(address who) external view returns (uint256);

  function allowance(address owner, address spender)
    external view returns (uint256);

  function transfer(address to, uint256 value) external returns (bool);

  function approve(address spender, uint256 value)
    external returns (bool);

  function transferFrom(address from, address to, uint256 value)
    external returns (bool);

  event Transfer(
    address indexed from,
    address indexed to,
    uint256 value
  );

  event Approval(
    address indexed owner,
    address indexed spender,
    uint256 value
  );
}

/**
 * @title SafeMath
 * @dev Math operations with safety checks that revert on error
 */
library SafeMath {

  /**
  * @dev Multiplies two numbers, reverts on overflow.
  */
  function mul(uint256 a, uint256 b) internal pure returns (uint256) {
    // Gas optimization: this is cheaper than requiring 'a' not being zero, but the
    // benefit is lost if 'b' is also tested.
    // See: OpenZeppelin/openzeppelin-contracts#522
    if (a == 0) {
      return 0;
    }

    uint256 c = a * b;
    require(c / a == b);

    return c;
  }

  /**
  * @dev Integer division of two numbers truncating the quotient, reverts on division by zero.
  */
  function div(uint256 a, uint256 b) internal pure returns (uint256) {
    require(b > 0); // Solidity only automatically asserts 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;
  }

  /**
  * @dev Subtracts two numbers, reverts on overflow (i.e. if subtrahend is greater than minuend).
  */
  function sub(uint256 a, uint256 b) internal pure returns (uint256) {
    require(b <= a);
    uint256 c = a - b;

    return c;
  }

  /**
  * @dev Adds two numbers, reverts on overflow.
  */
  function add(uint256 a, uint256 b) internal pure returns (uint256) {
    uint256 c = a + b;
    require(c >= a);

    return c;
  }

  /**
  * @dev Divides two numbers and returns the remainder (unsigned integer modulo),
  * reverts when dividing by zero.
  */
  function mod(uint256 a, uint256 b) internal pure returns (uint256) {
    require(b != 0);
    return a % b;
  }
}

/**
 * @title Standard ERC20 token
 *
 * @dev Implementation of the basic standard token.
 * https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md
 * Originally based on code by FirstBlood: https://github.com/Firstbloodio/token/blob/master/smart_contract/FirstBloodToken.sol
 */
contract ERC20 is IERC20 {
  using SafeMath for uint256;

  mapping (address => uint256) private _balances;

  mapping (address => mapping (address => uint256)) private _allowed;

  uint256 private _totalSupply;

  /**
  * @dev Total number of tokens in existence
  */
  function totalSupply() public view returns (uint256) {
    return _totalSupply;
  }

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

  /**
   * @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
    view
    returns (uint256)
  {
    return _allowed[owner][spender];
  }

  /**
  * @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) {
    _transfer(msg.sender, 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:
   * ethereum/EIPs#20 (comment)
   * @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) {
    require(spender != address(0));

    _allowed[msg.sender][spender] = value;
    emit Approval(msg.sender, spender, value);
    return true;
  }

  /**
   * @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(value <= _allowed[from][msg.sender]);

    _allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value);
    _transfer(from, to, value);
    return true;
  }

  /**
   * @dev Increase the amount of tokens that an owner allowed to a spender.
   * approve should be called when allowed_[_spender] == 0. To increment
   * allowed value is better to use this function to avoid 2 calls (and wait until
   * the first transaction is mined)
   * From MonolithDAO Token.sol
   * @param spender The address which will spend the funds.
   * @param addedValue The amount of tokens to increase the allowance by.
   */
  function increaseAllowance(
    address spender,
    uint256 addedValue
  )
    public
    returns (bool)
  {
    require(spender != address(0));

    _allowed[msg.sender][spender] = (
      _allowed[msg.sender][spender].add(addedValue));
    emit Approval(msg.sender, spender, _allowed[msg.sender][spender]);
    return true;
  }

  /**
   * @dev Decrease the amount of tokens that an owner allowed to a spender.
   * approve should be called when allowed_[_spender] == 0. To decrement
   * allowed value is better to use this function to avoid 2 calls (and wait until
   * the first transaction is mined)
   * From MonolithDAO Token.sol
   * @param spender The address which will spend the funds.
   * @param subtractedValue The amount of tokens to decrease the allowance by.
   */
  function decreaseAllowance(
    address spender,
    uint256 subtractedValue
  )
    public
    returns (bool)
  {
    require(spender != address(0));

    _allowed[msg.sender][spender] = (
      _allowed[msg.sender][spender].sub(subtractedValue));
    emit Approval(msg.sender, spender, _allowed[msg.sender][spender]);
    return true;
  }

  /**
  * @dev Transfer token for a specified addresses
  * @param from The address to transfer from.
  * @param to The address to transfer to.
  * @param value The amount to be transferred.
  */
  function _transfer(address from, address to, uint256 value) internal {
    require(value <= _balances[from]);
    require(to != address(0));

    _balances[from] = _balances[from].sub(value);
    _balances[to] = _balances[to].add(value);
    emit Transfer(from, to, value);
  }

  /**
   * @dev Internal function that mints an amount of the token and assigns it to
   * an account. This encapsulates the modification of balances such that the
   * proper events are emitted.
   * @param account The account that will receive the created tokens.
   * @param value The amount that will be created.
   */
  function _mint(address account, uint256 value) internal {
    require(account != 0);
    _totalSupply = _totalSupply.add(value);
    _balances[account] = _balances[account].add(value);
    emit Transfer(address(0), account, value);
  }

  /**
   * @dev Internal function that burns an amount of the token of a given
   * account.
   * @param account The account whose tokens will be burnt.
   * @param value The amount that will be burnt.
   */
  function _burn(address account, uint256 value) internal {
    require(account != 0);
    require(value <= _balances[account]);

    _totalSupply = _totalSupply.sub(value);
    _balances[account] = _balances[account].sub(value);
    emit Transfer(account, address(0), value);
  }

  /**
   * @dev Internal function that burns an amount of the token of a given
   * account, deducting from the sender's allowance for said account. Uses the
   * internal burn function.
   * @param account The account whose tokens will be burnt.
   * @param value The amount that will be burnt.
   */
  function _burnFrom(address account, uint256 value) internal {
    require(value <= _allowed[account][msg.sender]);

    // Should OpenZeppelin/openzeppelin-contracts#707 be accepted,
    // this function needs to emit an event with the updated approval.
    _allowed[account][msg.sender] = _allowed[account][msg.sender].sub(
      value);
    _burn(account, value);
  }
}


/**
 * @title ERC20Detailed token
 * @dev The decimals are only for visualization purposes.
 * All the operations are done using the smallest and indivisible token unit,
 * just as on Ethereum all the operations are done in wei.
 */
contract ERC20Detailed is IERC20 {
  string private _name;
  string private _symbol;
  uint8 private _decimals;

  constructor(string name, string symbol, uint8 decimals) public {
    _name = name;
    _symbol = symbol;
    _decimals = decimals;
  }

  /**
   * @return the name of the token.
   */
  function name() public view returns(string) {
    return _name;
  }

  /**
   * @return the symbol of the token.
   */
  function symbol() public view returns(string) {
    return _symbol;
  }

  /**
   * @return the number of decimals of the token.
   */
  function decimals() public view returns(uint8) {
    return _decimals;
  }
}



/**
 * @title Roles
 * @dev Library for managing addresses assigned to a Role.
 */
library Roles {
  struct Role {
    mapping (address => bool) bearer;
  }

  /**
   * @dev give an account access to this role
   */
  function add(Role storage role, address account) internal {
    require(account != address(0));
    require(!has(role, account));

    role.bearer[account] = true;
  }

  /**
   * @dev remove an account's access to this role
   */
  function remove(Role storage role, address account) internal {
    require(account != address(0));
    require(has(role, account));

    role.bearer[account] = false;
  }

  /**
   * @dev check if an account has this role
   * @return bool
   */
  function has(Role storage role, address account)
    internal
    view
    returns (bool)
  {
    require(account != address(0));
    return role.bearer[account];
  }
}

contract MinterRole {
  using Roles for Roles.Role;

  event MinterAdded(address indexed account);
  event MinterRemoved(address indexed account);

  Roles.Role private minters;

  constructor() internal {
    _addMinter(msg.sender);
  }

  modifier onlyMinter() {
    require(isMinter(msg.sender));
    _;
  }

  function isMinter(address account) public view returns (bool) {
    return minters.has(account);
  }

  function addMinter(address account) public onlyMinter {
    _addMinter(account);
  }

  function renounceMinter() public {
    _removeMinter(msg.sender);
  }

  function _addMinter(address account) internal {
    minters.add(account);
    emit MinterAdded(account);
  }

  function _removeMinter(address account) internal {
    minters.remove(account);
    emit MinterRemoved(account);
  }
}

/**
 * @title ERC20Mintable
 * @dev ERC20 minting logic
 */
contract ERC20Mintable is ERC20, MinterRole {
  /**
   * @dev Function to mint tokens
   * @param to The address that will receive the minted tokens.
   * @param value The amount of tokens to mint.
   * @return A boolean that indicates if the operation was successful.
   */
  function mint(
    address to,
    uint256 value
  )
    public
    onlyMinter
    returns (bool)
  {
    _mint(to, value);
    return true;
  }
}


/**
 * @title Burnable Token
 * @dev Token that can be irreversibly burned (destroyed).
 */
contract ERC20Burnable is ERC20 {

  /**
   * @dev Burns a specific amount of tokens.
   * @param value The amount of token to be burned.
   */
  function burn(uint256 value) public {
    _burn(msg.sender, value);
  }

  /**
   * @dev Burns a specific amount of tokens from the target address and decrements allowance
   * @param from address The address which you want to send tokens from
   * @param value uint256 The amount of token to be burned
   */
  function burnFrom(address from, uint256 value) public {
    _burnFrom(from, value);
  }
}




contract PauserRole {
  using Roles for Roles.Role;

  event PauserAdded(address indexed account);
  event PauserRemoved(address indexed account);

  Roles.Role private pausers;

  constructor() internal {
    _addPauser(msg.sender);
  }

  modifier onlyPauser() {
    require(isPauser(msg.sender));
    _;
  }

  function isPauser(address account) public view returns (bool) {
    return pausers.has(account);
  }

  function addPauser(address account) public onlyPauser {
    _addPauser(account);
  }

  function renouncePauser() public {
    _removePauser(msg.sender);
  }

  function _addPauser(address account) internal {
    pausers.add(account);
    emit PauserAdded(account);
  }

  function _removePauser(address account) internal {
    pausers.remove(account);
    emit PauserRemoved(account);
  }
}

/**
 * @title Pausable
 * @dev Base contract which allows children to implement an emergency stop mechanism.
 */
contract Pausable is PauserRole {
  event Paused(address account);
  event Unpaused(address account);

  bool private _paused;

  constructor() internal {
    _paused = false;
  }

  /**
   * @return true if the contract is paused, false otherwise.
   */
  function paused() public view returns(bool) {
    return _paused;
  }

  /**
   * @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() public onlyPauser whenNotPaused {
    _paused = true;
    emit Paused(msg.sender);
  }

  /**
   * @dev called by the owner to unpause, returns to normal state
   */
  function unpause() public onlyPauser whenPaused {
    _paused = false;
    emit Unpaused(msg.sender);
  }
}

/**
 * @title Pausable token
 * @dev ERC20 modified with pausable transfers.
 **/
contract ERC20Pausable is ERC20, 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 increaseAllowance(
    address spender,
    uint addedValue
  )
    public
    whenNotPaused
    returns (bool success)
  {
    return super.increaseAllowance(spender, addedValue);
  }

  function decreaseAllowance(
    address spender,
    uint subtractedValue
  )
    public
    whenNotPaused
    returns (bool success)
  {
    return super.decreaseAllowance(spender, subtractedValue);
  }
}


contract PayUSD is ERC20, ERC20Detailed, ERC20Mintable, ERC20Burnable, ERC20Pausable {

    address public staker;
    uint256 public transferFee = 10; // 0.001 % if feeDenominator = 10000
    uint256 public mintFee = 10; // 0.001 % if feeDenominator = 10000
    uint256 public feeDenominator = 10000;

    event ChangeStaker(address indexed addr);
    event ChangeStakingFees (uint256 transferFee, uint256 mintFee, uint256 feeDenominator);

    constructor(address _staker)
        ERC20Burnable()
        ERC20Mintable()
        ERC20Pausable()
        ERC20Detailed("PayUSD", "PUSD", 18)
        ERC20()
        public {
        staker = _staker;
    }

    function mint(address to, uint256 value) public onlyMinter returns (bool) {
        bool result = super.mint(to, value);
        payStakingFee(to, value, mintFee);
        return result;
    }


    function transfer(address to, uint256 value) public returns (bool) {
        bool result = super.transfer(to, value);
        payStakingFee(to, value, transferFee);
        return result;   
    }

    function transferFrom(address from, address to, uint256 value) public returns (bool) {
        bool result = super.transferFrom(from, to, value);
        payStakingFee(to, value, transferFee);
        return result;
    }


    function payStakingFee(address _payer, uint256 _value, uint256 _fees) internal {
        uint256 stakingFee = _value.mul(_fees).div(feeDenominator);
        _transfer(_payer, staker, stakingFee);
    }

    function changeStakingFees(uint256 _transferFee,uint256 _mintFee, uint256 _feeDenominator) public onlyMinter {        
        require(transferFee < feeDenominator, "_feeDenominator must be greater than _transferFee");
        require(mintFee < feeDenominator, "_feeDenominator must be greater than _mintFee");

        transferFee = _transferFee; 
        mintFee = _mintFee; 
        feeDenominator = _feeDenominator;
        
        emit ChangeStakingFees(
            transferFee,
            mintFee,
            feeDenominator
        );
    }

    function changeStaker(address _newStaker) public onlyMinter {
        require(_newStaker != address(0), "new staker cannot be 0x0");
        staker = _newStaker;
        emit ChangeStaker(_newStaker);
    }
}
@DevCon1100

This comment was marked as spam.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nventuro nventuro

Labels
contracts Smart contract code.
Projects
None yet
Milestone
v2.1
5 participants
@frangio @shrugs @nventuro @DevCon1100 and others