ERC777.sol: _callTokensToSend should be called after _move, not before, to follow Checks-Effects-Interactions

[lukehutch]

EIP-777 says "The tokensToSend hook MUST be called before the state is updated—i.e. before the balance is decremented."

However, this violates Checks-Effects-Interactions, and introduces a very real risk of vulnerability for any contract that implements ERC777.

In this (now-closed) OpenZeppelin kitchen-sink security issue, this potential vulnerability is discussed, and it was agreed that probably the ERC777 standard should not be followed in this instance: @frangio says, agreeing with @guylando and @nventuro:

#1749 (comment)

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

However this was never actually changed in the OpenZeppelin implementation of ERC777:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC777/ERC777.sol#L376
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC777/ERC777.sol#L400

This leaves any contract overriding OpenZeppelin's ERC777 implementation potentially vulnerable to exploitation via sender hook.

[frangio]

I'm not sure there is anything we can do about this. The EIP is very clear about the order in which things should be done. And other smart contracts are written with this assumption in mind, e.g.:

openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol

Lines 175 to 180 in 4135bf9

	// If _asset is ERC777, `transferFrom` can trigger a reenterancy BEFORE the transfer happens through the
	// `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
	// calls the vault, which is assumed not malicious.
	//
	// Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
	// assets are transfered and before the shares are minted, which is a valid state.

Honestly at this point the best option might be #2620.

[mpereiraesaa]

Modifying the implementation of an EIP basically breaking some standard is never the way to go even if that way we might fix some bad behavior. In any case it can be avoided or deprecated like suggested by @frangio .

We should always write smart contracts understanding the risks of some standards while trying to integrate a token.

[yashdeharia]

@mpereiraesaa ... you are right

[lukehutch]

@mpereiraesaa @frangio It looks to me that ERC1363 and ERC4524 are much better (simpler and more robust) standards. Maybe OpenZeppelin should replace the ERC777 implementation with those? I'll close this bug and move the discussion to #2620, thanks.