From 538655c3c06b615143141552b2d6d6f8515e4499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 3 May 2023 09:35:48 +0200 Subject: [PATCH] Add reentrancy test cases for ERC4626 (#4197) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- contracts/mocks/ERC20Reentrant.sol | 43 +++++ test/token/ERC20/extensions/ERC4626.test.js | 175 ++++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 contracts/mocks/ERC20Reentrant.sol diff --git a/contracts/mocks/ERC20Reentrant.sol b/contracts/mocks/ERC20Reentrant.sol new file mode 100644 index 000000000..c0184b77b --- /dev/null +++ b/contracts/mocks/ERC20Reentrant.sol @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import "../token/ERC20/ERC20.sol"; +import "../token/ERC20/extensions/ERC4626.sol"; + +contract ERC20Reentrant is ERC20("TEST", "TST") { + enum Type { + No, + Before, + After + } + + Type private _reenterType; + address private _reenterTarget; + bytes private _reenterData; + + function scheduleReenter(Type when, address target, bytes calldata data) external { + _reenterType = when; + _reenterTarget = target; + _reenterData = data; + } + + function functionCall(address target, bytes memory data) public returns (bytes memory) { + return Address.functionCall(target, data); + } + + function _beforeTokenTransfer(address from, address to, uint256 amount) internal override { + if (_reenterType == Type.Before) { + _reenterType = Type.No; + functionCall(_reenterTarget, _reenterData); + } + super._beforeTokenTransfer(from, to, amount); + } + + function _afterTokenTransfer(address from, address to, uint256 amount) internal override { + super._afterTokenTransfer(from, to, amount); + if (_reenterType == Type.After) { + _reenterType = Type.No; + functionCall(_reenterTarget, _reenterData); + } + } +} diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index ee0998717..55b3e5d20 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -1,11 +1,14 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); +const { Enum } = require('../../../helpers/enums'); + const ERC20Decimals = artifacts.require('$ERC20DecimalsMock'); const ERC4626 = artifacts.require('$ERC4626'); const ERC4626OffsetMock = artifacts.require('$ERC4626OffsetMock'); const ERC4626FeesMock = artifacts.require('$ERC4626FeesMock'); const ERC20ExcessDecimalsMock = artifacts.require('ERC20ExcessDecimalsMock'); +const ERC20Reentrant = artifacts.require('$ERC20Reentrant'); contract('ERC4626', function (accounts) { const [holder, recipient, spender, other, user1, user2] = accounts; @@ -44,6 +47,178 @@ contract('ERC4626', function (accounts) { } }); + describe('reentrancy', async function () { + const reenterType = Enum('No', 'Before', 'After'); + + const amount = web3.utils.toBN(1000000000000000000); + const reenterAmount = web3.utils.toBN(1000000000); + let token; + let vault; + + beforeEach(async function () { + token = await ERC20Reentrant.new(); + // Use offset 1 so the rate is not 1:1 and we can't possibly confuse assets and shares + vault = await ERC4626OffsetMock.new('', '', token.address, 1); + // Funds and approval for tests + await token.$_mint(holder, amount); + await token.$_mint(other, amount); + await token.$_approve(holder, vault.address, constants.MAX_UINT256); + await token.$_approve(other, vault.address, constants.MAX_UINT256); + await token.$_approve(token.address, vault.address, constants.MAX_UINT256); + }); + + // During a `_deposit`, the vault does `transferFrom(depositor, vault, assets)` -> `_mint(receiver, shares)` + // such that a reentrancy BEFORE the transfer guarantees the price is kept the same. + // If the order of transfer -> mint is changed to mint -> transfer, the reentrancy could be triggered on an + // intermediate state in which the ratio of assets/shares has been decreased (more shares than assets). + it('correct share price is observed during reentrancy before deposit', async function () { + // mint token for deposit + await token.$_mint(token.address, reenterAmount); + + // Schedules a reentrancy from the token contract + await token.scheduleReenter( + reenterType.Before, + vault.address, + vault.contract.methods.deposit(reenterAmount, holder).encodeABI(), + ); + + // Initial share price + const sharesForDeposit = await vault.previewDeposit(amount, { from: holder }); + const sharesForReenter = await vault.previewDeposit(reenterAmount, { from: holder }); + + // Do deposit normally, triggering the _beforeTokenTransfer hook + const receipt = await vault.deposit(amount, holder, { from: holder }); + + // Main deposit event + await expectEvent(receipt, 'Deposit', { + sender: holder, + owner: holder, + assets: amount, + shares: sharesForDeposit, + }); + // Reentrant deposit event → uses the same price + await expectEvent(receipt, 'Deposit', { + sender: token.address, + owner: holder, + assets: reenterAmount, + shares: sharesForReenter, + }); + + // Assert prices is kept + const sharesAfter = await vault.previewDeposit(amount, { from: holder }); + expect(sharesForDeposit).to.be.bignumber.eq(sharesAfter); + }); + + // During a `_withdraw`, the vault does `_burn(owner, shares)` -> `transfer(receiver, assets)` + // such that a reentrancy AFTER the transfer guarantees the price is kept the same. + // If the order of burn -> transfer is changed to transfer -> burn, the reentrancy could be triggered on an + // intermediate state in which the ratio of shares/assets has been decreased (more assets than shares). + it('correct share price is observed during reentrancy after withdraw', async function () { + // Deposit into the vault: holder gets `amount` share, token.address gets `reenterAmount` shares + await vault.deposit(amount, holder, { from: holder }); + await vault.deposit(reenterAmount, token.address, { from: other }); + + // Schedules a reentrancy from the token contract + await token.scheduleReenter( + reenterType.After, + vault.address, + vault.contract.methods.withdraw(reenterAmount, holder, token.address).encodeABI(), + ); + + // Initial share price + const sharesForWithdraw = await vault.previewWithdraw(amount, { from: holder }); + const sharesForReenter = await vault.previewWithdraw(reenterAmount, { from: holder }); + + // Do withdraw normally, triggering the _afterTokenTransfer hook + const receipt = await vault.withdraw(amount, holder, holder, { from: holder }); + + // Main withdraw event + await expectEvent(receipt, 'Withdraw', { + sender: holder, + receiver: holder, + owner: holder, + assets: amount, + shares: sharesForWithdraw, + }); + // Reentrant withdraw event → uses the same price + await expectEvent(receipt, 'Withdraw', { + sender: token.address, + receiver: holder, + owner: token.address, + assets: reenterAmount, + shares: sharesForReenter, + }); + + // Assert price is kept + const sharesAfter = await vault.previewWithdraw(amount, { from: holder }); + expect(sharesForWithdraw).to.be.bignumber.eq(sharesAfter); + }); + + // Donate newly minted tokens to the vault during the reentracy causes the share price to increase. + // Still, the deposit that trigger the reentracy is not affected and get the previewed price. + // Further deposits will get a different price (getting fewer shares for the same amount of assets) + it('share price change during reentracy does not affect deposit', async function () { + // Schedules a reentrancy from the token contract that mess up the share price + await token.scheduleReenter( + reenterType.Before, + token.address, + token.contract.methods.$_mint(vault.address, reenterAmount).encodeABI(), + ); + + // Price before + const sharesBefore = await vault.previewDeposit(amount); + + // Deposit, triggering the _beforeTokenTransfer hook + const receipt = await vault.deposit(amount, holder, { from: holder }); + + // Price is as previewed + await expectEvent(receipt, 'Deposit', { + sender: holder, + owner: holder, + assets: amount, + shares: sharesBefore, + }); + + // Price was modified during reentrancy + const sharesAfter = await vault.previewDeposit(amount); + expect(sharesAfter).to.be.bignumber.lt(sharesBefore); + }); + + // Burn some tokens from the vault during the reentracy causes the share price to drop. + // Still, the withdraw that trigger the reentracy is not affected and get the previewed price. + // Further withdraw will get a different price (needing more shares for the same amount of assets) + it('share price change during reentracy does not affect withdraw', async function () { + await vault.deposit(amount, other, { from: other }); + await vault.deposit(amount, holder, { from: holder }); + + // Schedules a reentrancy from the token contract that mess up the share price + await token.scheduleReenter( + reenterType.After, + token.address, + token.contract.methods.$_burn(vault.address, reenterAmount).encodeABI(), + ); + + // Price before + const sharesBefore = await vault.previewWithdraw(amount); + + // Withdraw, triggering the _afterTokenTransfer hook + const receipt = await vault.withdraw(amount, holder, holder, { from: holder }); + + // Price is as previewed + await expectEvent(receipt, 'Withdraw', { + sender: holder, + receiver: holder, + owner: holder, + assets: amount, + shares: sharesBefore, + }); + + // Price was modified during reentrancy + const sharesAfter = await vault.previewWithdraw(amount); + expect(sharesAfter).to.be.bignumber.gt(sharesBefore); + }); + }); + for (const offset of [0, 6, 18].map(web3.utils.toBN)) { const parseToken = token => web3.utils.toBN(10).pow(decimals).muln(token); const parseShare = share => web3.utils.toBN(10).pow(decimals.add(offset)).muln(share);