diff --git a/CHANGELOG.md b/CHANGELOG.md index d3d794b4026..872190b7968 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ * `SafeMath`: fix a memory allocation issue by adding new `SafeMath.tryOp(uint,uint)→(bool,uint)` functions. `SafeMath.op(uint,uint,string)→uint` are now deprecated. ([#2462](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2462)) * `EnumerableMap`: fix a memory allocation issue by adding new `EnumerableMap.tryGet(uint)→(bool,address)` functions. `EnumerableMap.get(uint)→string` is now deprecated. ([#2462](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2462)) +### Security Fixes + + * `ERC777`: fix potential reentrancy issues for custom extensions to `ERC777`. ([#2483](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2483)) + +If you're using our implementation of ERC777 from version 3.3.0 or earlier, and you define a custom `_beforeTokenTransfer` function that writes to a storage variable, you may be vulnerable to a reentrancy attack. If you're affected and would like assistance please write to security@openzeppelin.com. [Read more in the pull request.](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2483) + ## 3.3.0 (2020-11-26) * Now supports both Solidity 0.6 and 0.7. Compiling with solc 0.7 will result in warnings. Install the `solc-0.7` tag to compile without warnings. diff --git a/contracts/mocks/ERC777Mock.sol b/contracts/mocks/ERC777Mock.sol index 757e6597ca5..0916d484762 100644 --- a/contracts/mocks/ERC777Mock.sol +++ b/contracts/mocks/ERC777Mock.sol @@ -6,6 +6,8 @@ import "../utils/Context.sol"; import "../token/ERC777/ERC777.sol"; contract ERC777Mock is Context, ERC777 { + event BeforeTokenTransfer(); + constructor( address initialHolder, uint256 initialBalance, @@ -28,4 +30,8 @@ contract ERC777Mock is Context, ERC777 { function approveInternal(address holder, address spender, uint256 value) public { _approve(holder, spender, value); } + + function _beforeTokenTransfer(address, address, address, uint256) internal override { + emit BeforeTokenTransfer(); + } } diff --git a/contracts/mocks/ERC777SenderRecipientMock.sol b/contracts/mocks/ERC777SenderRecipientMock.sol index 43fdf3bfaee..186d8f82ae5 100644 --- a/contracts/mocks/ERC777SenderRecipientMock.sol +++ b/contracts/mocks/ERC777SenderRecipientMock.sol @@ -34,6 +34,9 @@ contract ERC777SenderRecipientMock is Context, IERC777Sender, IERC777Recipient, uint256 toBalance ); + // Emitted in ERC777Mock. Here for easier decoding + event BeforeTokenTransfer(); + bool private _shouldRevertSend; bool private _shouldRevertReceive; diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 248936ac436..b28a8602ce2 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -390,10 +390,10 @@ contract ERC777 is Context, IERC777, IERC20 { address operator = _msgSender(); - _beforeTokenTransfer(operator, from, address(0), amount); - _callTokensToSend(operator, from, address(0), amount, data, operatorData); + _beforeTokenTransfer(operator, from, address(0), amount); + // Update state variables _balances[from] = _balances[from].sub(amount, "ERC777: burn amount exceeds balance"); _totalSupply = _totalSupply.sub(amount); diff --git a/test/token/ERC777/ERC777.test.js b/test/token/ERC777/ERC777.test.js index 61eb6d4b24a..9f247d2529e 100644 --- a/test/token/ERC777/ERC777.test.js +++ b/test/token/ERC777/ERC777.test.js @@ -447,4 +447,37 @@ contract('ERC777', function (accounts) { expect(await this.token.defaultOperators()).to.deep.equal([]); }); }); + + describe('relative order of hooks', function () { + beforeEach(async function () { + await singletons.ERC1820Registry(registryFunder); + this.sender = await ERC777SenderRecipientMock.new(); + await this.sender.registerRecipient(this.sender.address); + await this.sender.registerSender(this.sender.address); + this.token = await ERC777.new(holder, initialSupply, name, symbol, []); + await this.token.send(this.sender.address, 1, '0x', { from: holder }); + }); + + it('send', async function () { + const { receipt } = await this.sender.send(this.token.address, anyone, 1, '0x'); + + const internalBeforeHook = receipt.logs.findIndex(l => l.event === 'BeforeTokenTransfer'); + expect(internalBeforeHook).to.be.gte(0); + const externalSendHook = receipt.logs.findIndex(l => l.event === 'TokensToSendCalled'); + expect(externalSendHook).to.be.gte(0); + + expect(externalSendHook).to.be.lt(internalBeforeHook); + }); + + it('burn', async function () { + const { receipt } = await this.sender.burn(this.token.address, 1, '0x'); + + const internalBeforeHook = receipt.logs.findIndex(l => l.event === 'BeforeTokenTransfer'); + expect(internalBeforeHook).to.be.gte(0); + const externalSendHook = receipt.logs.findIndex(l => l.event === 'TokensToSendCalled'); + expect(externalSendHook).to.be.gte(0); + + expect(externalSendHook).to.be.lt(internalBeforeHook); + }); + }); });