Add `tokensToSend` Callback
0xjac opened this issue · 2 comments
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.
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.
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.