Add reentrancy test cases for ERC4626 (#4197)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>

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);
+        }
+    }
+}

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);