diff --git a/.changeset/blue-scissors-design.md b/.changeset/blue-scissors-design.md new file mode 100644 index 00000000000..c2f815aae4f --- /dev/null +++ b/.changeset/blue-scissors-design.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Math`: Make `ceilDiv` to revert on 0 division even if the numerator is 0 diff --git a/contracts/mocks/token/ERC20Reentrant.sol b/contracts/mocks/token/ERC20Reentrant.sol index ee803b9e1a5..00ba7426087 100644 --- a/contracts/mocks/token/ERC20Reentrant.sol +++ b/contracts/mocks/token/ERC20Reentrant.sol @@ -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 { diff --git a/contracts/mocks/token/ERC4626LimitsMock.sol b/contracts/mocks/token/ERC4626LimitsMock.sol new file mode 100644 index 00000000000..a47826bb864 --- /dev/null +++ b/contracts/mocks/token/ERC4626LimitsMock.sol @@ -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; + } +} diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index a2fc7ceb76f..d372295d7a8 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -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; + } + // (a + b - 1) / b can overflow on addition, so we distribute. return a == 0 ? 0 : (a - 1) / b + 1; } diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index 5e87f8d5272..1693c4aa697 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -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); diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index d67486a60fc..99d6009e4dc 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -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'); @@ -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); @@ -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'); } @@ -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'); } @@ -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'); @@ -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'); } @@ -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 } @@ -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 @@ -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'); } @@ -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'); } @@ -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'); } @@ -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 } diff --git a/test/token/ERC721/extensions/ERC721Burnable.test.js b/test/token/ERC721/extensions/ERC721Burnable.test.js index c6c0769191a..df059e09078 100644 --- a/test/token/ERC721/extensions/ERC721Burnable.test.js +++ b/test/token/ERC721/extensions/ERC721Burnable.test.js @@ -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); @@ -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', [ diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 172da86a667..55c26dffe08 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -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 () { @@ -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', + [], + ); + }); }); }); } diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index df459c5f82c..afd822b1788 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -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'); @@ -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'); @@ -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'); diff --git a/test/utils/structs/Checkpoints.test.js b/test/utils/structs/Checkpoints.test.js index d8218012778..936ac565af6 100644 --- a/test/utils/structs/Checkpoints.test.js +++ b/test/utils/structs/Checkpoints.test.js @@ -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'); @@ -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), @@ -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'); @@ -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()); });