NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Implement recommendations from 5.0 audit Phase 1A (#4398)

Browse Source 
Co-authored-by: Francisco Giordano <fg@frang.io>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
This commit is contained in:
Ernesto García
2023-07-03 12:02:06 -06:00
committed by GitHub
parent 06861dce54
commit bb64458928
38 changed files with 779 additions and 666 deletions
Download Patch File Download Diff File Expand all files Collapse all files

110
test/token/ERC20/extensions/ERC4626.test.js
View File

@ -52,8 +52,8 @@ 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);
const value = web3.utils.toBN(1000000000000000000);
const reenterValue = web3.utils.toBN(1000000000);
let token;
let vault;
@ -62,8 +62,8 @@ contract('ERC4626', function (accounts) {
// 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.$_mint(holder, value);
await token.$_mint(other, value);
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);
@ -75,39 +75,39 @@ contract('ERC4626', function (accounts) {
// 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);
await token.$_mint(token.address, reenterValue);
// Schedules a reentrancy from the token contract
await token.scheduleReenter(
reenterType.Before,
vault.address,
vault.contract.methods.deposit(reenterAmount, holder).encodeABI(),
vault.contract.methods.deposit(reenterValue, holder).encodeABI(),
);
// Initial share price
const sharesForDeposit = await vault.previewDeposit(amount, { from: holder });
const sharesForReenter = await vault.previewDeposit(reenterAmount, { from: holder });
const sharesForDeposit = await vault.previewDeposit(value, { from: holder });
const sharesForReenter = await vault.previewDeposit(reenterValue, { from: holder });
// Do deposit normally, triggering the _beforeTokenTransfer hook
const receipt = await vault.deposit(amount, holder, { from: holder });
const receipt = await vault.deposit(value, holder, { from: holder });
// Main deposit event
await expectEvent(receipt, 'Deposit', {
sender: holder,
owner: holder,
assets: amount,
assets: value,
shares: sharesForDeposit,
});
// Reentrant deposit event → uses the same price
await expectEvent(receipt, 'Deposit', {
sender: token.address,
owner: holder,
assets: reenterAmount,
assets: reenterValue,
shares: sharesForReenter,
});
// Assert prices is kept
const sharesAfter = await vault.previewDeposit(amount, { from: holder });
const sharesAfter = await vault.previewDeposit(value, { from: holder });
expect(sharesForDeposit).to.be.bignumber.eq(sharesAfter);
});
@ -116,30 +116,30 @@ contract('ERC4626', function (accounts) {
// 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 });
// Deposit into the vault: holder gets `value` share, token.address gets `reenterValue` shares
await vault.deposit(value, holder, { from: holder });
await vault.deposit(reenterValue, 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(),
vault.contract.methods.withdraw(reenterValue, holder, token.address).encodeABI(),
);
// Initial share price
const sharesForWithdraw = await vault.previewWithdraw(amount, { from: holder });
const sharesForReenter = await vault.previewWithdraw(reenterAmount, { from: holder });
const sharesForWithdraw = await vault.previewWithdraw(value, { from: holder });
const sharesForReenter = await vault.previewWithdraw(reenterValue, { from: holder });
// Do withdraw normally, triggering the _afterTokenTransfer hook
const receipt = await vault.withdraw(amount, holder, holder, { from: holder });
const receipt = await vault.withdraw(value, holder, holder, { from: holder });
// Main withdraw event
await expectEvent(receipt, 'Withdraw', {
sender: holder,
receiver: holder,
owner: holder,
assets: amount,
assets: value,
shares: sharesForWithdraw,
});
// Reentrant withdraw event → uses the same price
@ -147,76 +147,76 @@ contract('ERC4626', function (accounts) {
sender: token.address,
receiver: holder,
owner: token.address,
assets: reenterAmount,
assets: reenterValue,
shares: sharesForReenter,
});
// Assert price is kept
const sharesAfter = await vault.previewWithdraw(amount, { from: holder });
const sharesAfter = await vault.previewWithdraw(value, { 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)
// Further deposits will get a different price (getting fewer shares for the same value 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(),
token.contract.methods.$_mint(vault.address, reenterValue).encodeABI(),
);
// Price before
const sharesBefore = await vault.previewDeposit(amount);
const sharesBefore = await vault.previewDeposit(value);
// Deposit, triggering the _beforeTokenTransfer hook
const receipt = await vault.deposit(amount, holder, { from: holder });
const receipt = await vault.deposit(value, holder, { from: holder });
// Price is as previewed
await expectEvent(receipt, 'Deposit', {
sender: holder,
owner: holder,
assets: amount,
assets: value,
shares: sharesBefore,
});
// Price was modified during reentrancy
const sharesAfter = await vault.previewDeposit(amount);
const sharesAfter = await vault.previewDeposit(value);
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)
// Further withdraw will get a different price (needing more shares for the same value 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 });
await vault.deposit(value, other, { from: other });
await vault.deposit(value, 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(),
token.contract.methods.$_burn(vault.address, reenterValue).encodeABI(),
);
// Price before
const sharesBefore = await vault.previewWithdraw(amount);
const sharesBefore = await vault.previewWithdraw(value);
// Withdraw, triggering the _afterTokenTransfer hook
const receipt = await vault.withdraw(amount, holder, holder, { from: holder });
const receipt = await vault.withdraw(value, holder, holder, { from: holder });
// Price is as previewed
await expectEvent(receipt, 'Withdraw', {
sender: holder,
receiver: holder,
owner: holder,
assets: amount,
assets: value,
shares: sharesBefore,
});
// Price was modified during reentrancy
const sharesAfter = await vault.previewWithdraw(amount);
const sharesAfter = await vault.previewWithdraw(value);
expect(sharesAfter).to.be.bignumber.gt(sharesBefore);
});
});
@ -738,9 +738,9 @@ contract('ERC4626', function (accounts) {
describe('ERC4626Fees', function () {
const feeBasePoint = web3.utils.toBN(5e3);
const amountWithoutFees = web3.utils.toBN(10000);
const fees = amountWithoutFees.mul(feeBasePoint).divn(1e5);
const amountWithFees = amountWithoutFees.add(fees);
const valueWithoutFees = web3.utils.toBN(10000);
const fees = valueWithoutFees.mul(feeBasePoint).divn(1e5);
const valueWithFees = valueWithoutFees.add(fees);
describe('input fees', function () {
beforeEach(async function () {
@ -760,13 +760,13 @@ contract('ERC4626', function (accounts) {
});
it('deposit', async function () {
expect(await this.vault.previewDeposit(amountWithFees)).to.be.bignumber.equal(amountWithoutFees);
({ tx: this.tx } = await this.vault.deposit(amountWithFees, recipient, { from: holder }));
expect(await this.vault.previewDeposit(valueWithFees)).to.be.bignumber.equal(valueWithoutFees);
({ tx: this.tx } = await this.vault.deposit(valueWithFees, recipient, { from: holder }));
});
it('mint', async function () {
expect(await this.vault.previewMint(amountWithoutFees)).to.be.bignumber.equal(amountWithFees);
({ tx: this.tx } = await this.vault.mint(amountWithoutFees, recipient, { from: holder }));
expect(await this.vault.previewMint(valueWithoutFees)).to.be.bignumber.equal(valueWithFees);
({ tx: this.tx } = await this.vault.mint(valueWithoutFees, recipient, { from: holder }));
});
afterEach(async function () {
@ -774,7 +774,7 @@ contract('ERC4626', function (accounts) {
await expectEvent.inTransaction(this.tx, this.token, 'Transfer', {
from: holder,
to: this.vault.address,
value: amountWithFees,
value: valueWithFees,
});
// redirect fees
@ -788,15 +788,15 @@ contract('ERC4626', function (accounts) {
await expectEvent.inTransaction(this.tx, this.vault, 'Transfer', {
from: constants.ZERO_ADDRESS,
to: recipient,
value: amountWithoutFees,
value: valueWithoutFees,
});
// deposit event
await expectEvent.inTransaction(this.tx, this.vault, 'Deposit', {
sender: holder,
owner: recipient,
assets: amountWithFees,
shares: amountWithoutFees,
assets: valueWithFees,
shares: valueWithoutFees,
});
});
});
@ -819,13 +819,13 @@ contract('ERC4626', function (accounts) {
});
it('redeem', async function () {
expect(await this.vault.previewRedeem(amountWithFees)).to.be.bignumber.equal(amountWithoutFees);
({ tx: this.tx } = await this.vault.redeem(amountWithFees, recipient, holder, { from: holder }));
expect(await this.vault.previewRedeem(valueWithFees)).to.be.bignumber.equal(valueWithoutFees);
({ tx: this.tx } = await this.vault.redeem(valueWithFees, recipient, holder, { from: holder }));
});
it('withdraw', async function () {
expect(await this.vault.previewWithdraw(amountWithoutFees)).to.be.bignumber.equal(amountWithFees);
({ tx: this.tx } = await this.vault.withdraw(amountWithoutFees, recipient, holder, { from: holder }));
expect(await this.vault.previewWithdraw(valueWithoutFees)).to.be.bignumber.equal(valueWithFees);
({ tx: this.tx } = await this.vault.withdraw(valueWithoutFees, recipient, holder, { from: holder }));
});
afterEach(async function () {
@ -833,7 +833,7 @@ contract('ERC4626', function (accounts) {
await expectEvent.inTransaction(this.tx, this.token, 'Transfer', {
from: this.vault.address,
to: recipient,
value: amountWithoutFees,
value: valueWithoutFees,
});
// redirect fees
@ -847,7 +847,7 @@ contract('ERC4626', function (accounts) {
await expectEvent.inTransaction(this.tx, this.vault, 'Transfer', {
from: holder,
to: constants.ZERO_ADDRESS,
value: amountWithFees,
value: valueWithFees,
});
// withdraw event
@ -855,8 +855,8 @@ contract('ERC4626', function (accounts) {
sender: holder,
receiver: recipient,
owner: holder,
assets: amountWithoutFees,
shares: amountWithFees,
assets: valueWithoutFees,
shares: valueWithFees,
});
});
});