Skip to content

Commit

Permalink
Change behavior of ceilDiv(0, 0) and improve test coverage (#4348)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ernestognw
ernestognw authored Jun 14, 2023
1 parent ac5480e commit 2477534
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/blue-scissors-design.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Math`: Make `ceilDiv` to revert on 0 division even if the numerator is 0
2 changes: 1 addition & 1 deletion contracts/mocks/token/ERC20Reentrant.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.19;

import "../../token/ERC20/ERC20.sol";
import "../../token/ERC20/extensions/ERC4626.sol";
import "../../utils/Address.sol";

contract ERC20Reentrant is ERC20("TEST", "TST") {
enum Type {
Expand Down
23 changes: 23 additions & 0 deletions contracts/mocks/token/ERC4626LimitsMock.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import "../../token/ERC20/extensions/ERC4626.sol";

abstract contract ERC4626LimitsMock is ERC4626 {
uint256 _maxDeposit;
uint256 _maxMint;

constructor() {
_maxDeposit = 100 ether;
_maxMint = 100 ether;
}

function maxDeposit(address) public view override returns (uint256) {
return _maxDeposit;
}

function maxMint(address) public view override returns (uint256) {
return _maxMint;
}
}
5 changes: 5 additions & 0 deletions contracts/utils/math/Math.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ library Math {
* of rounding down.
*/
function ceilDiv(uint256 a, uint256 b) internal pure returns (uint256) {
if (b == 0) {
// Guarantee the same behavior as in a regular Solidity division.
return a / b;

This comment has been minimized.

Sign in to view

Copy link
@pcaversaccio

pcaversaccio Jun 16, 2023 •
edited
Loading

Contributor

I have a quick question here: This will revert with the error Panic(0x12). As long as you have an environment that knows how to decode what this error means (i.e. "If you divide or modulo by zero") or you're native enough to know what it means it's not an issue. But what if you have a sequence of complex interactions and maybe need some further information to understand where the panic comes from. Don't you think the following would make more sense?

/**
 * @dev Error that occurs when dividing by zero in the function `ceilDiv`.
 * @param emitter The contract that emits the error.
 */
error CeilDivDivisionByZero(address emitter);

...
if (b == 0) revert CeilDivDivisionByZero(address(this));

This comment has been minimized.

Sign in to view

Copy link
@Amxx

Amxx Jun 16, 2023

Collaborator

My personal opinion is that all divisions by 0 should behave the same. If you emit a custom error here, but solidity still does Panic(0x12), then you have an inconsistency.

Also, we decided not to add the address of the custom error emitter in the custom error. If you remove that parameter, the value over a panic(0x12) is unclear to me.

Another relevant part of out library:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/Math.sol#L144-L155

In the first part we panic, in the second part ... maybe we want to panic instead of throwing the error is denominator is 0.

This comment has been minimized.

Sign in to view

Copy link
@pcaversaccio

pcaversaccio Jun 16, 2023

Contributor

right, agreed on the consistency part. For the other part of the library you're referring to, it's slightly subtle imo:

  • You could have two panics overall: 0x11 (overflow) or 0x12 (division by zero) removing that check;
    BUT, since it's wrapped in an unchecked block 0x11 won't throw and division by zero for denominator == 0 will not throw in mulmod since it's inline-assembly where division by 0 returns 0. So the custom error might be the only good solution here.

This comment has been minimized.

Sign in to view

Copy link
@Amxx

Amxx Jun 16, 2023

Collaborator

or ethereum/solidity#11792

This comment has been minimized.

Sign in to view

Copy link
@pcaversaccio

pcaversaccio Jun 16, 2023

Contributor

oh yeah - that would be the best solution indeed.
}

// (a + b - 1) / b can overflow on addition, so we distribute.
return a == 0 ? 0 : (a - 1) / b + 1;
}
Expand Down
8 changes: 8 additions & 0 deletions test/token/ERC1155/ERC1155.behavior.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,14 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m
);
});

it('reverts when transferring from zero address', async function () {
await expectRevertCustomError(
this.token.$_safeBatchTransferFrom(ZERO_ADDRESS, multiTokenHolder, [firstTokenId], [firstAmount], '0x'),
'ERC1155InvalidSender',
[ZERO_ADDRESS],
);
});

function batchTransferWasSuccessful({ operator, from, ids, values }) {
it('debits transferred balances from sender', async function () {
const newBalances = await this.token.balanceOfBatch(new Array(ids.length).fill(from), ids);
Expand Down
74 changes: 74 additions & 0 deletions test/token/ERC20/extensions/ERC4626.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { expectRevertCustomError } = require('../../../helpers/customError');

const ERC20Decimals = artifacts.require('$ERC20DecimalsMock');
const ERC4626 = artifacts.require('$ERC4626');
const ERC4626LimitsMock = artifacts.require('$ERC4626LimitsMock');
const ERC4626OffsetMock = artifacts.require('$ERC4626OffsetMock');
const ERC4626FeesMock = artifacts.require('$ERC4626FeesMock');
const ERC20ExcessDecimalsMock = artifacts.require('ERC20ExcessDecimalsMock');
Expand Down Expand Up @@ -220,6 +221,49 @@ contract('ERC4626', function (accounts) {
});
});

describe('limits', async function () {
beforeEach(async function () {
this.token = await ERC20Decimals.new(name, symbol, decimals);
this.vault = await ERC4626LimitsMock.new(name + ' Vault', symbol + 'V', this.token.address);
});

it('reverts on deposit() above max deposit', async function () {
const maxDeposit = await this.vault.maxDeposit(holder);
await expectRevertCustomError(this.vault.deposit(maxDeposit.addn(1), recipient), 'ERC4626ExceededMaxDeposit', [
recipient,
maxDeposit.addn(1),
maxDeposit,
]);
});

it('reverts on mint() above max mint', async function () {
const maxMint = await this.vault.maxMint(holder);
await expectRevertCustomError(this.vault.mint(maxMint.addn(1), recipient), 'ERC4626ExceededMaxMint', [
recipient,
maxMint.addn(1),
maxMint,
]);
});

it('reverts on withdraw() above max withdraw', async function () {
const maxWithdraw = await this.vault.maxWithdraw(holder);
await expectRevertCustomError(
this.vault.withdraw(maxWithdraw.addn(1), recipient, holder),
'ERC4626ExceededMaxWithdraw',
[holder, maxWithdraw.addn(1), maxWithdraw],
);
});

it('reverts on redeem() above max redeem', async function () {
const maxRedeem = await this.vault.maxRedeem(holder);
await expectRevertCustomError(
this.vault.redeem(maxRedeem.addn(1), recipient, holder),
'ERC4626ExceededMaxRedeem',
[holder, maxRedeem.addn(1), maxRedeem],
);
});
});

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);
Expand Down Expand Up @@ -849,6 +893,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('0');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('2000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('0');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'2000',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('2000');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('2000');
}
Expand All @@ -872,6 +919,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('4000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('2000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('4000');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'6000',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('6000');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('6000');
}
Expand All @@ -883,6 +933,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('4000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('2999'); // used to be 3000, but virtual assets/shares captures part of the yield
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('5999'); // used to be 6000, but virtual assets/shares captures part of the yield
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'6000',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('6000');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('9000');

Expand All @@ -904,6 +957,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('4000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('4999');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('6000');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'7333',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('7333');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('11000');
}
Expand All @@ -928,6 +984,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('6000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('4999'); // used to be 5000
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('9000');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'9333',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('9333');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('14000'); // used to be 14001
}
Expand All @@ -940,6 +999,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('6000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('6070'); // used to be 6071
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('10928'); // used to be 10929
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'9333',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('9333');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('17000'); // used to be 17001

Expand All @@ -961,6 +1023,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('6000');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('3643');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('10929');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'8000',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('8000');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('14573');
}
Expand All @@ -983,6 +1048,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('4392');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('3643');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('8000');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'6392',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('6392');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('11644');
}
Expand All @@ -1006,6 +1074,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('4392');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('0');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('8000'); // used to be 8001
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'4392',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('4392');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('8001');
}
Expand All @@ -1028,6 +1099,9 @@ contract('ERC4626', function (accounts) {
expect(await this.vault.balanceOf(user2)).to.be.bignumber.equal('0');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user1))).to.be.bignumber.equal('0');
expect(await this.vault.convertToAssets(await this.vault.balanceOf(user2))).to.be.bignumber.equal('0');
expect(await this.vault.convertToShares(await this.token.balanceOf(this.vault.address))).to.be.bignumber.equal(
'0',
);
expect(await this.vault.totalSupply()).to.be.bignumber.equal('0');
expect(await this.vault.totalAssets()).to.be.bignumber.equal('1'); // used to be 0
}
Expand Down
11 changes: 10 additions & 1 deletion test/token/ERC721/extensions/ERC721Burnable.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const { expectRevertCustomError } = require('../../../helpers/customError');
const ERC721Burnable = artifacts.require('$ERC721Burnable');

contract('ERC721Burnable', function (accounts) {
const [owner, approved] = accounts;
const [owner, approved, another] = accounts;

const firstTokenId = new BN(1);
const secondTokenId = new BN(2);
Expand Down Expand Up @@ -61,6 +61,15 @@ contract('ERC721Burnable', function (accounts) {
});
});

describe('when there is no previous approval burned', function () {
it('reverts', async function () {
await expectRevertCustomError(this.token.burn(tokenId, { from: another }), 'ERC721InsufficientApproval', [
another,
tokenId,
]);
});
});

describe('when the given token ID was not tracked by this contract', function () {
it('reverts', async function () {
await expectRevertCustomError(this.token.burn(unknownTokenId, { from: owner }), 'ERC721NonexistentToken', [
Expand Down
19 changes: 19 additions & 0 deletions test/token/ERC721/extensions/ERC721Consecutive.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ contract('ERC721Consecutive', function (accounts) {
expect(await this.token.getVotes(account)).to.be.bignumber.equal(web3.utils.toBN(balance));
}
});

it('reverts on consecutive minting to the zero address', async function () {
await expectRevertCustomError(
ERC721ConsecutiveMock.new(name, symbol, offset, delegates, [ZERO_ADDRESS], [10]),
'ERC721InvalidReceiver',
[ZERO_ADDRESS],
);
});
});

describe('minting after construction', function () {
Expand Down Expand Up @@ -172,6 +180,17 @@ contract('ERC721Consecutive', function (accounts) {
expect(await this.token.$_exists(tokenId)).to.be.equal(true);
expect(await this.token.ownerOf(tokenId), user2);
});

it('reverts burning batches of size != 1', async function () {
const tokenId = batches[0].amount + offset;
const receiver = batches[0].receiver;

await expectRevertCustomError(
this.token.$_afterTokenTransfer(receiver, ZERO_ADDRESS, tokenId, 2),
'ERC721ForbiddenBatchBurn',
[],
);
});
});
});
}
Expand Down
18 changes: 18 additions & 0 deletions test/utils/math/Math.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const { BN, constants, expectRevert } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');
const { MAX_UINT256 } = constants;
const { Rounding } = require('../../helpers/enums.js');
const { expectRevertCustomError } = require('../../helpers/customError.js');

const Math = artifacts.require('$Math');

Expand Down Expand Up @@ -204,6 +205,19 @@ contract('Math', function () {
});

describe('ceilDiv', function () {
it('reverts on zero division', async function () {
const a = new BN('2');
const b = new BN('0');
// It's unspecified because it's a low level 0 division error
await expectRevert.unspecified(this.math.$ceilDiv(a, b));
});

it('does not round up a zero result', async function () {
const a = new BN('0');
const b = new BN('2');
expect(await this.math.$ceilDiv(a, b)).to.be.bignumber.equal('0');
});

it('does not round up on exact division', async function () {
const a = new BN('10');
const b = new BN('5');
Expand Down Expand Up @@ -233,6 +247,10 @@ contract('Math', function () {
await expectRevert.unspecified(this.math.$mulDiv(1, 1, 0, Rounding.Down));
});

it('reverts with result higher than 2 ^ 256', async function () {
await expectRevertCustomError(this.math.$mulDiv(5, MAX_UINT256, 2, Rounding.Down), 'MathOverflowedMulDiv', []);
});

describe('does round down', async function () {
it('small values', async function () {
expect(await this.math.$mulDiv('3', '4', '5', Rounding.Down)).to.be.bignumber.equal('2');
Expand Down
15 changes: 15 additions & 0 deletions test/utils/structs/Checkpoints.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const { expect } = require('chai');

const { VALUE_SIZES } = require('../../../scripts/generate/templates/Checkpoints.opts.js');
const { expectRevertCustomError } = require('../../helpers/customError.js');
const { expectRevert } = require('@openzeppelin/test-helpers');

const $Checkpoints = artifacts.require('$Checkpoints');

Expand All @@ -22,6 +23,7 @@ contract('Checkpoints', function () {
describe(`Trace${length}`, function () {
beforeEach(async function () {
this.methods = {
at: (...args) => this.mock.methods[`$at_${libraryName}_Trace${length}(uint256,uint32)`](0, ...args),
latest: (...args) => this.mock.methods[`$latest_${libraryName}_Trace${length}(uint256)`](0, ...args),
latestCheckpoint: (...args) =>
this.mock.methods[`$latestCheckpoint_${libraryName}_Trace${length}(uint256)`](0, ...args),
Expand All @@ -35,6 +37,11 @@ contract('Checkpoints', function () {
});

describe('without checkpoints', function () {
it('at zero reverts', async function () {
// Reverts with array out of bound access, which is unspecified
await expectRevert.unspecified(this.methods.at(0));
});

it('returns zero as latest value', async function () {
expect(await this.methods.latest()).to.be.bignumber.equal('0');

Expand Down Expand Up @@ -65,6 +72,14 @@ contract('Checkpoints', function () {
}
});

it('at keys', async function () {
for (const [index, { key, value }] of this.checkpoints.entries()) {
const at = await this.methods.at(index);
expect(at._value).to.be.bignumber.equal(value);
expect(at._key).to.be.bignumber.equal(key);
}
});

it('length', async function () {
expect(await this.methods.length()).to.be.bignumber.equal(this.checkpoints.length.toString());
});
Expand Down

0 comments on commit 2477534

Please sign in to comment.