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.

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

Add tokensToSend Callback #23

Closed
0xjac opened this issue Feb 6, 2018 · 2 comments
Closed

Add tokensToSend Callback #23

0xjac opened this issue Feb 6, 2018 · 2 comments
Assignees

Comments

@0xjac
Copy link
Owner

0xjac commented Feb 6, 2018

interface ERC777TokensSender {
    function tokensToSend(
        address operator,
        address from,
        address to,
        uint value,
        bytes userData,
        bytes operatorData
    ) public;
}

This proposal adds the above interface which must be registered via EIP-820, similar to tokensReceived.
The difference from tokensReceived is that tokensToSend is called for the from address before sending the tokens. This allows the token holder to perform a a final check, logging or some other action before sending the tokens.

@0xjac
Copy link
Owner Author

0xjac commented Feb 6, 2018

Deviating from send:

One of the main reason of changing the method name from transfer to send was
to have a method to send tokens similar to the send for ether, with the tokensReceived to notify the recipient contract and act as a fallback function. Adding a widely different behavior such as tokenToSend goes against that intention. send for tokens should be as close as possible to send for ether.

Added complexity

Understanding what happens when sending tokens is more complicated. Now two methods must be checked and understood.

Adoption of tokensToSend

I am not sure many people will actually use tokensToSend. I fear implementing it will add some features only for a few people while most will just ignore it anyway.

Purpose of tokensToSend and tokensReceived

The main idea behind tokensReceived was to notify contracts of received funds. A throw to refuse tokens is also a good use of that function. However implementing checks is not the first intended goal of that function.
On the other hand, this is the intended goal of tokensToSend, and I am not sure this is the best place to put those checks. Operators are better suited (see below).

Relative usefulness of self imposed checks

All the checks in the tokensToSend are self imposed by the token sender (from) and do not add extra security. A sender can decide to just not implement the checks or remove them at any time.

Operators are more powerful and more flexible

Any functionality implemented in tokensToSend can be implemented using an operator.

Considering the following token holder:

contract TokenHolder {
  function tokensReceived(...) {}

  function tokensToSend(...) { require(customCheck()); }
}

Simply move the check into the operator:

contract Operator {
  function send(...) {
    require(customCheck());
    Ierc777(token).operatorSend(...);
  }
}

And then always call Operator.send(). There is no more guarantee that a user will remove tokenSender checks than he will bypass an operator.

For the case of operators, the standard could be amended to consider valid token contracts with the following constraints:

  • only operatorSend, prevent the user to send directly
  • no one can be the operator of himself (operatorSend must be used at all time) - operator must be a contract: thus users must explicitly set a blank operator to allow the regular behavior
  • optionally, operator must be whitelisted to only allow certain behaviors.
    This essential relaxes some aspects of the standard and does not add any extra logic. The existing behavior is preserved but those specific and strict uses cases can also be handled.

One fear is the difficulty for wallets to implement those operators but I fear that it will be harder and less likely for regular users to implement tokensToSend correctly and deploy it. A nice solution to this would be to provide operators (outside the standard) implementing the most common use cases:

  • rate limiting/time-block

    contract TimeLockOperator {
    
        address private tokenContract;
    
        mapping(address => address) private dispatchers;
        mapping(address => address) private recipients
    
        function TimeLockOperator(address _tokenContract) public {
            tokenContract = _tokenContract;
        }
    
        function triggerTimer(address to) public {
            allowed[msg.sender][to] = block.timestamp + 1 hours;
        }
    
        function timeLockedSend(address _to, uint256 _amount, bytes _data) public {
            require(allowed[msg.sender][_to] >= block.timestamp);
            allowed[from][to] = 0;
            Ierc777(tokenContract).operatorSend(msg.sender, _to, _amount, "", _data);
        }
    }
  • blacklist addresses: cold wallet to only hot wallet transfers

    contract FixedRecipientOperator {
    
        address private tokenContract;
    
        mapping(address => address) private dispatchers;
        mapping(address => address) private recipients
    
        function FixedRecipientOperator(address _tokenContract) public {
            tokenContract = _tokenContract;
        }
    
        function registerDispatcher(address _dispatcher, address _recipient) public {
            dispatchers[msg.sender] = _dispatcher;
            recipients[msg.sender] = _recipient;
        }
    
        function refillAccount(address _from, uint256 _amount, bytes data) public {
            require(dispatchers[_from] == msg.sender);
            Ierc777(tokenContract).operatorSend(_from, recipients[_from], _amount, "", data);
        }
    }

    This contract could be deployed once and used by anyone. Furthermore the _from, _dispatcher and _recipient do not need to be contracts either. A regular user would not need to deploy any contract.

    I think this offers an interesting scenario where one can store a large amount of tokens in a cold wallet and give the private key of the dispatcher for safe keeping.
    The worst case scenario if the dispatcher's key is compromised is sending all the tokens to the intended recipient. And if the cold wallet's key is lost, the dispatcher can still be used to recover all the funds from the cold wallet.

  • quotas

  • eagerly send tokens before a send for taxes/fees (I actually don't see how to do this with tokensToSend without having recursion issues)

Wallets can simply use those known operators in their interface and regular users can easily enable them by authorizing them without having to write and deploy a contract.

Implementing the same logic but with common TokenHolder is more difficult because the user can only have one TokenHolder at any time and this TokenHolder will also be implementing tokensReceived which may be problematic.

ERC-20 backwards compatibility with operators

tokensToSend is executed every time tokens are sent, this include ERC-20 methods (transfer and transferFrom) Using operators to perform checks is based on the use of operatorSend. One may think that to send tokens with checks to a legacy contract requires bypassing the operator and use approve / transferFrom. There is a very simple way around it. The legacy contract should register a proxy ITokenRecipient contract, then operatorSend can be used directly. Let's assume however that the legacy contract does not register a proxy ITokenRecipient. An operator can still be used to transfer tokens to a legacy contract with one slight extension of the token contract: Add a operatorAllow(address from, address to, uint256 amount) to the operator contract. This is an operator function which allows an authorized operator to call allow to a legacy contract to on behalf of from. In this scenario, the custom checks must happen in the operator right before it calls the operatorAllow on the token contract.

Conflicting pattern and risks of re-entry

Using tokensToSend tries to notify someone before checks and effects. This apparently breaks the Checks-Effects-Interactions pattern.

Intuitively token developers could assume tokensToSend must be called before updating the balances. However this opens the door to re entry attacks. I fear this may be a common bug which may break many contracts.

Internally, token developers must imperatively call tokensToSend after updating the balances and before calling tokensReceived otherwise this opens the door the double-spend attacks via re-entry. This will respect the CEI pattern but does imply tokensToSend is a notification while what is does is really some external checks afterwards. This can be confusing as to the expected role of tokensToSend.


In conclusion, I think a much simpler solution is to slightly relax the standard if strict checks (more than what tokensToSend can provide) are needed. And more importantly, deploy implementations of operators with the most common use cases. This will allow people wanting checks to use them even if they lack the technical ability to write a contract.

It also keeps the standard simple, the behavior predictable and does not weaken the security.

If self imposed checks to legacy contracts not implementing a proxy contract for ERC-777 turns out to be a highly sought after feature, I would add the operatorAllow method as part of the standard for ERC-20 compatible implementations.

@0xjac 0xjac changed the title Replace ITokenRecipient with ITokenHolder Add tokensToSend Callback Feb 22, 2018
@0xjac 0xjac self-assigned this Feb 22, 2018
@0xjac
Copy link
Owner Author

0xjac commented Feb 22, 2018

Regarding my previous comments and after discussing the matter internally. I've revised my position.

Using EIP-820 (which is already in used by ERC-777) does not significantly add more complexity.

There are some valid use cases for having this check in place; and those cannot be implemented with operators in scenarios where some safety checks must be implemented for all operator transactions.

The ability of changing an address's manager in EIP-820 also ensure that even with the private key of the account, the check cannot be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants