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

Fix ERC777 potential reentrancy issues #2483

Merged
merged 4 commits into from
Jan 26, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix relative order of hooks in ERC777.burn
  • Loading branch information
frangio committed Jan 20, 2021
commit 61bf5b6b3f8c3eba47d5b0923a5f9fe53bfeaac1
6 changes: 6 additions & 0 deletions contracts/mocks/ERC777Mock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 operator, address from, address to, uint256 amount) internal override {
emit BeforeTokenTransfer();
}
}
3 changes: 3 additions & 0 deletions contracts/mocks/ERC777SenderRecipientMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions contracts/token/ERC777/ERC777.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
33 changes: 33 additions & 0 deletions test/token/ERC777/ERC777.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -447,4 +447,37 @@ contract('ERC777', function (accounts) {
expect(await this.token.defaultOperators()).to.deep.equal([]);
});
});

describe.only('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);
});
});
});