ERC777 implementation and security clarifications

[guylando]

Those are merely questions and clarifications to better understand the decisions behind the ERC777 implementation. Please don't consider them as critique. We are looking to use your ERC777 implementation code and just want to be confident about it and to make sure any problems, if there are any, are discovered/discussed/understood as early as possible.

1. (UPDATE: answered by nventuro in the comments - requires changes)In transferFrom why is _transfer called before lowering allowance and _approve? https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC777/ERC777.sol#L123
   I checked that the same happens in ERC20. Doesn't it sound safer to first adjust allowance and only then perform the transfer (external call)? (Checks-Effects-Interactions pattern)

2. (UPDATE: answered by mcdee in the comments)Why burn doesn't have limited access (token creator only)? why to allow anybody to burn their tokens? it influences the token economics https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC777/ERC777.sol#L134
   maybe by default to let nobody (or only token creator) burn tokens and to add "Burnable" interface which adds the ability for anybody to burn tokens?

3. (UPDATE: answered by mcdee in the comments)Why burning tokens is done by sending them to address(0) instead of some other way (such as a "burnedTokens" counter)? The discussions in Reclaiming of ether in common classes of stuck accounts ethereum/EIPs#156 and in a few other places show that many people mistakenly sent tokens to address(0) and are looking for ways to now recover them. So knowingly to continue sending tokens to address(0) by design for burning purposes seems undesirable, no?

4. (UPDATE: answered by mcdee in the comments)IERC1820Registry (0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24) and TOKENS_SENDER_INTERFACE_HASH (0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895) and TOKENS_RECIPIENT_INTERFACE_HASH (0xb281fc8c12954d22544db45de3159a39272895b169a852b314f9cc762e44c53b) and "ERC777Token" and "ERC20Token" are all hardcoded into the token contract without a way to modify them. Is that a good idea? We are all aware that new critical bugs are discovered in contracts very often, some because of solidity modifications or new eips and some because of new attack ideas. Redeploying a token in a new address (redeploying in an old address using things like CREATE2 losses trust) requires a lot of explanations to the users who own the token or who are looking into buying the token. So maybe there should have been provided some alternative secure mechanism instead of hard-coding those things just in case something changes?

5. (UPDATE: answered by mcdee in the comments)How do the calls to _callTokensReceived and _callTokensToSend go hand in hand with the Checks-Effects-Interactions pattern? For example in the _send function _callTokensToSend is called before balances modifications. Same in _burn and _mint.
   In this article by OpenZeppelin CTO: https://blog.zeppelin.solutions/onward-with-ethereum-smart-contract-security-97a827e47702
   in the "Order your function code: conditions, actions, interactions" section he gives an example where the event emitting is inserted under "2. Effects" before the "3. Interactions". So why _send, _burn, _mint emit the events in the end of the code?

6. (UPDATE: answered by nventuro in the comments)Since _totalSupply is private and is not set/increased anywhere other than in the internal _mint, it means that initial supply is expected to be set by an inheriting contract in the constructor by calling _mint?

7. (UPDATE2: answered by nventuro in the comments)(UPDATE: I see the validation in _callTokensReceived does not apply to the ERC20 functions which pass "false" to the validation boolean. Can you confirm it that this makes ERC777 work with multisig wallets which support ERC20?) The implementation of _callTokensReceived forces receiver to either be an EOA or to implement ERC1820/ERC165. So what happens for multisig wallets? The most popular of them at this time is gnosis. Will it not be able to hold the ERC777 tokens? https://github.com/gnosis/MultiSigWallet/blob/master/contracts/MultiSigWallet.sol I am not sure that even the newer gnosis safe implements that https://github.com/gnosis/safe-contracts/tree/development/contracts
   since multisig wallets are important for holding the initial tokens funds, this seems like a critical matter for them to support the ERC777 implementation or otherwise for security reasons, it will be better for token creators to create ERC20 supported by multisig wallets than to create ERC777 without support from multisig wallets.

8. (UPDATE: answered by nventuro in the comments)_callTokensReceived uses extcodesize to decide if the address is a contract so if _mint is called at the constructor of the contract then the test will pass (since extcodesize returns 0 at the constructor) and the contract will be able to get tokens while additional tokens after the contract creation will not be able to be sent to it (using the ERC777 new functions) if it doesn't implement the desired interface.
   https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/utils/Address.sol#L23
   https://ethereum.stackexchange.com/questions/15641/how-does-a-contract-find-out-if-another-address-is-a-contract/15642#15642

[mcdee]

I can answer a few of these questions as apply to the spec rather than the implementation.

2. Any token holder can effectively burn tokens by sending them to a known inaccessible address (e.g. 0xdead). However, actually burning tokens can and does reduce total supply as this is logically consistent with burning.

3. ERC-777 specifically disallows sending of tokens to 0x00.

4. The hashes are identifiers and there is no point at which they would need to be changed. The ERC-1820 address is a known address; if ERC-1820 were found to have issues a new standard could supersede it but there is no guarantee it would be directly compatible with ERC-1820 and as such it would most likely require a separate token contract rather than a simple change of address.

5. total supply should be decremented when burning tokens.

[guylando]

@mcdee Thanks for your comment.

2. Would a "forced burn" (sending to 0xdead) have same psychological effect on the market and economics as an explicit burn using burn function which decreases total supply and makes EVERYBODY aware that a burn occurred?
3. I speak about the implementation of _burn function which sends the tokens to 0x00 https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC777/ERC777.sol#L390
   as compared to other possible implementations which could just decrease total supply and user balance without calling _callTokensToSend and Transfer event. Why does _burn call _callTokensToSend and Transfer event?
4. You are saying that if a bug is found in ERC-1820 registry contract then ALL ERC777 tokens would be required to redeploy? This would cause a chaos to token holders (especially non-technical ones) as compared to providing some upgradability behavior in regards to the ERC-1820 address. I understand your argument however I think a more common un-addressed scenario is a simple backward-compatible bugfix upgrade required for ERC-1820 contract which will not break all ERC777 contracts.
5. I am not sure what did you address by this statement. If you speak about _burn implementation then my question was why is _callTokensToSend called before total supply and balance changes (violating the Effects-Interactions pattern it seems).

[mcdee]

2. Providing a bun() function that decreases total supply is a better solution as total supply simply states the actual total supply. I'm unqualified to comment on the psychological effect on the market.

3. That function does not send tokens to 0x00. If it did it would not be ERC-777 compliant. Calling tokensToSend() and emitting Transfer() are parts of the spec, but neither of these send tokens to 0x00.

4. No, I'm saying that superseding ERC-1820 would make no difference to existing contracts as they are already registered with ERC-1820. Just updating the address of the registry contract would not be effective because there would need to be a full update process (unregistering from ERC-1820, re-registering with the new registry contract, having to check both old and new registries for implementers, handling clashes) for it to continue to work. Making the registry address variable does not help with these implementation details

The item labeled 5 should have been for 6; apologies.

[guylando]

Thanks for your answers,

3. By "sending to 0x00" I refer to the call of "_callTokensToSend" and the emission of "Transfer" event (which, because of the naming, make the code reader interpret it as "sending to 0x00" I guess). Since the spec is at a very early stage, of course I am hoping that there is a good explanation for everything and that there are 0 problems and 0 vulnerabilities, but I still ask the questions even if the spec claims its "ok" just to make sure. In this audit I was trying to understand the reasoning behind the implementation/spec including things like this (which can introduce problems for coders which would expect _callTokensToSend or Transfer to be called only for transfers because of the naming. They are not called "_callTokensToSendOrBurn" "TransferOrBurn"). Also I read in the spec that it says on the one hand that tokensToSend can block the sending operation using revert and on the other hand "The tokensToSend hook MUST be called before the state is updated—i.e. before the balance is decremented". Why does the spec force the tokensToSend hook be called before the state is updated if the revert will revert the state change anyway? might introduce reentrance issues by calling external contract before state modifications, don't you think? If the reason for the external contract to have access to state BEFORE the changes then it could be achieved by passing that state as parameter to the hook or by assuming in the hook that state was modified and subtract the appropriate value to get the previous state if it is necessary for the hook calculations.

4. Do you refer to a bugfix in the ERC-1820 registry contract (pointed by the hardcoded address in ERC777 open-zepplin implementation) as "superseding ERC-1820"?

6. You are right, changed my wording from "modified" to "set/increased" now. The question is still valid.

--

Another point arose after reading https://www.wealdtech.com/articles/understanding-erc777-token-contracts/ and https://www.wealdtech.com/articles/understanding-erc777-token-operator-contracts/:

9. (UPDATE: answered by mcdee in the comments)Basically from attacker perspective the _defaultOperators allows a full backdoor on the token in the sense that for any token buyer to trust a token they have to audit any contract listed in _defaultOperators and not only the token contract itself, correct?

Note1: Also "token operator contract can be used with any ERC-777 token and only a single copy needs to be deployed on the blockchain" introduces even more risk in the sense that many tokens creators just copy mindlessly other contracts code which can make one vulnerability in some token operator contract affect multiple deployed ERC777 tokens which use the contract. In this perspective it is great that there is a way to revoke default operators however if an attacker will find a vulnerability in an operator then he might utilize the exploit simultaneously on all affected tokens before they have a chance to revoke him. This DOES increase the attack surface by encouraging different tokens to use common operators contracts. I guess maybe the benefits are bigger than the risk in the minds of the creators. I see the final statement in the second article confirms this: "Any weaknesses in token operator contracts can be magnified when the same token operator contract is used by multiple token contracts. Careful auditing of token operator contracts is highly encouraged.".

Note2: Some of my statements are based on my security research and security code review background (not specifically for the blockchain however security attacks in different fields have common grounds).

[mcdee]

3. The Transfer() event is for ERC-20 compatibility. A pure ERC-777 token would not emit this, as it has its own Burned() event that is more explicit.

tokensToSend() is a hook that acts before state changes, as per its name (and as opposed to tokensReceived() which is a hook after state changes). As such it can examine state (account balances) prior to the transfer and decide if the transfer is to proceed or not.

It is also important to understand that the contract in which tokensToSend() is called is specified by the token holder. As such a number of the guidelines around C-E-I can be relaxed (or alternatively you can think of tokensToSend() as part of the check phase; same end result).

4. any changes to ERC-1820 would cause a new contract to be deployed; there is no way of putting in changes and retaining the existing address.

As to your point 9), the short answer is "yes" but the important point is that due to the separation of concerns it is far easier to audit a few simple token operator contracts than an entire token contract as would be the case with ERC-20.

Also "token operator contract can be used with any ERC-777 token and only a single copy needs to be deployed on the blockchain" introduces even more risk in the sense that many tokens creators just copy mindlessly other contracts code which can make one vulnerability in some token operator contract affect multiple deployed ERC777 tokens which use the contract.

More risk in having a single, well-examined and audited contract than having everyone write their own? The purpose is to minimise risk, not to eliminate it. If people want to write their own token operator contracts they are of course welcome to, but allowing shared contracts provides an alternative that in the majority of cases will be safer for token holders.

[guylando]

4. You mean that redeploying ERC-1820 at a different address for a bugfix will have to introduce a new ERC number and will be considered "superseding ERC-1820"? It can't still be ERC-1820, just with a bugfixed new address redeployment? Because if it can then my argument about allowing upgradability for that contract address seems valid (by the way the new CREATE2 opcode CAN actually allow redeploying in same address as explained here: https://medium.com/@jason.carver/defend-against-wild-magic-in-the-next-ethereum-upgrade-b008247839d2 https://ethereum-magicians.org/t/potential-security-implications-of-create2-eip-1014/2614 , although it is unrelated to the discussion here).

[mcdee]

4. CREATE2 was not used to deploy ERC-1820 and cannot be used to redeploy it in case of changes. Any changes would make it something other than ERC-1820. And as above just changing the address of the registry within a token contract would end up with a broken implementation unless a number of additional steps were also taken by the token contract to migrate entirely to the new registry contract (and even then it would not remain an ERC-777 compliant contract, as ERC-1820 is mandated within the ERC-777 spec).

[guylando]

@mcdee Thanks for all the answers.

Regarding original questions just to clear out for anybody reading this, what is left unanswered are questions number: 1, 6, 7, 8 (updated others as "answered").

[nventuro]

Wow, this is a great discussion, thank you @guylando and @mcdee!

6. Since _totalSupply is private and is not set/increased anywhere other than in the internal _mint, it means that initial supply is expected to be set by an inheriting contract in the constructor by calling _mint?

Indeed, there's no concept of 'initial supply' in our implementation of ERC777, but if you do need an initial holder, calling _mint in the constructor will achieve that. You can read more about different supply mechanisms using our API in this forum post

7. (UPDATE: I see the validation in _callTokensReceived does not apply to the ERC20 functions which pass "false" to the validation boolean. Can you confirm it that this makes ERC777 work with multisig wallets which support ERC20?) The implementation of _callTokensReceived forces receiver to either be an EOA or to implement ERC1820/ERC165. So what happens for multisig wallets? The most popular of them at this time is gnosis. Will it not be able to hold the ERC777 tokens? https://github.com/gnosis/MultiSigWallet/blob/master/contracts/MultiSigWallet.sol I am not sure that even the newer gnosis safe implements that https://github.com/gnosis/safe-contracts/tree/development/contracts
   since multisig wallets are important for holding the initial tokens funds, this seems like a critical matter for them to support the ERC777 implementation or otherwise for security reasons, it will be better for token creators to create ERC20 supported by multisig wallets than to create ERC777 without support from multisig wallets.

Our ERC777 contract implements ERC20 compatiblity as described in the EIP, which relaxes the requirement on recipient contracts to implement the ERC777TokenReceiver, but only when using the transfer method. So yes, any contract can hold this ERC777, as long as transfer is used and not send.

8. _callTokensReceived uses extcodesize to decide if the address is a contract so if _mint is called at the constructor of the contract then the test will pass (since extcodesize returns 0 at the constructor) and the contract will be able to get tokens while additional tokens after the contract creation will not be able to be sent to it (using the ERC777 new functions) if it doesn't implement the desired interface.
   https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/utils/Address.sol#L23
   https://ethereum.stackexchange.com/questions/15641/how-does-a-contract-find-out-if-another-address-is-a-contract/15642#15642

This is 100% correct, but it's important to keep in mind that the purpose behind this requirement is to help avoid having tokens locked in contracts. A more thorough check is as far as I know not possible, and I think the EIP authors made the right call by using extcodesize.

1. In transferFrom why is _transfer called before lowering allowance and _approve? https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC777/ERC777.sol#L123
   I checked that the same happens in ERC20. Doesn't it sound safer to first adjust allowance and only then perform the transfer (external call)? (Checks-Effects-Interactions pattern)

This is actually a great point! ERC20 doesn't have this issue because transfer does not perform an external call, but this may happen here. I don't think this is a security concern, but the EIP specifies that tokensToSend must be called before the state is updated, and tokensReceived after. Our implementation does not currently adhere to this, at least when it comes to ERC20 allowances. @frangio do you agree with this interpretation of the EIP?

[frangio]

Yes, I agree, we're gonna have to change our implementation to update allowances before calling tokensReceived.

Thank you for the review @guylando! It helps a lot. Let us know if you have any more questions or comments.

[guylando]

No more questions or comments, although some of my points were partially answered or I do not agree with the decisions (in point 3 deciding if publicly allowing anybody to decrease total supply is maybe worse psychologically/economically than letting users send tokens to 0xdead, in point 4 I am not convinced that hardcoding the 1820 registry address is a good idea in light that any minor fix of 1820 which will be uploaded to a new address will force ALL ERC777 TOKENS which are based on openzeppelin to redeploy which is a mass).
Anyway my main goal was to raise awareness and to understand the decisions so this was achieved it seems so I am closing this unless you want to keep it open for the change you wrote that you are going to do.

[guylando]

Also regarding the decision on allowing what I wrote in point 8, need to put a lot of thought if this allows some exploit or not. Sounds dangerous but I have no example for an exploit (dangerous because it is the type of things that everybody can think is safe until there comes someone who thinks of some cool exploit which nobody thought about before. the exploit could utilize some existing ethereum/solidity behavior or some future behavior which will come in the future).

[nventuro]

I'm not sure how point 8 could lead to an exploint: there's no inherent issue with a contract that is not an implementer having tokens. Such a thing could be achieved without resorting to the constructor trick: a contract may register an implementer, receive the tokens, and then unregister it.

The intent behind this requirement is to prevent contracts that are unaware of 777's existence (and therefore lack any mechanisms to transfer the tokens out of these contracts) from receiving tokens in the first place, since they would be effectively burned, but it is still possible for a contract to implement the interface and have no way of transferring them, getting us back to square one. The requirement is not a security one, but an attempt to avoid unfortunate scenarios (that cannot be 100% avoided).

[guylando]

@frangio we want to use the ERC777 but waiting for the changes you wrote. Any idea when they will be merged to this repo or can you suggest on the exact changes which we should manually do so that our changes will be consistent with your changes?
Thanks

[frangio]

@guylando They will be available later today in a new release candidate. The final 2.3.0 release will be available later this week or early next week.

[nventuro]

@guylando you can install the latest release candidate, including these changes, by running

npm install openzeppelin-solidity@next

[guylando]

@nventuro Thanks!

1. Note that in https://github.com/OpenZeppelin/openzeppelin-solidity/releases/tag/v2.3.0-rc.3 the link to the code of ERC777 is outdated and does not point to the latest code.
2. I left some more comments about ERC777 code at Update transferFrom to modify allowance in-between hook calls. #1751:
   Update transferFrom to modify allowance in-between hook calls. #1751 (comment)
   Update transferFrom to modify allowance in-between hook calls. #1751 (comment)
   Update transferFrom to modify allowance in-between hook calls. #1751 (comment)

Anyway, good job on the quick implementation and fixes!

[frangio]

@guylando

By the way I don't see why would anyone want to create a non-ERC20 compatible ERC777 token in the near 6-12 months. [...] I would suggest not to waste time on that until ERC777 gets more support

Yeah this has been our thought process as well. We only made sure that a future opt-out API would be feasible in terms of backwards compatibility.

maybe check that address is not (case insensitive) 0xdcc703c0E500B653Ca82273B7BFAd8045D85a470 in addition to 0x0

This is interesting but we will not add this check. It does not belong at the smart contract level. We check for 0x0 because it's the default for uninitialized values like mapping entries.

isOperatorFor is used both for operatorSend and operatorBurn which seems surprising in the sense that if someone authorizes an operator to send his tokens, I am not sure he also wants to authorize the operator to burn his tokens (in most cases)

This is a concern of spec rather than the implementation, but in any case operatorSend allows sending the tokens to any irrecoverable address, so operatorBurn is only a natural consequence.

[guylando]

I believe there is a difference between sending to irrecoverable address and burning which decreases total supply because the first is not visible to the non-technical investors while the second one will change the token total supply on coinmarketcap which might cause undesired reaction from non-technical investors who follow the token in coinmarketcap and similar tools

[guylando]

Accidentally found a "proof" of the difference between burning and sending to an invalid address:
ethereum/EIPs#867 (comment)
ethereum/EIPs#867 (comment)
where the difference is enhanced in such discussions as of the retrieval of stuck tokens which wouldn't be discussed about burned tokens

[guylando]

So from legal point of view there is more claim to the owner of the tokens that he still owns legally the tokens which were sent to an invalid address than for him to claim he owns burned tokens.