Skip to content

Commit

Permalink
Custom Errors For ERC20 and unit tests
Browse files Browse the repository at this point in the history 
Fixes #2839

Add feature custom error to ERC20

Testing is raising errors for multicall, ERC777 and ERC 4626 because I have not added the feature to them yet, and they are using ERC20 behavior. Wanted to make sure the design decisions I made for the feature and testing are OK so I can move forward to the other contracts

- [ X ] Tests
- [ ] Documentation
- [ ] Changeset entry (run `npx changeset add`)
  • Loading branch information
@lhemerly
lhemerly committed Mar 26, 2023
1 parent 3f610eb commit bb8ba79
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 36 deletions.
85 changes: 74 additions & 11 deletions contracts/token/ERC20/ERC20.sol
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,42 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (token/ERC20/ERC20.sol)

pragma solidity ^0.8.0;
pragma solidity ^0.8.4;

import "./IERC20.sol";
import "./extensions/IERC20Metadata.sol";
import "../../utils/Context.sol";

/**
* @dev Implementation of solidity 0.8.4 custom errors.
*
* This implementation adopted the convention of errors before and outside the contract
* code.
*
* The following are EIP-6093 implementations:
* https://eips.ethereum.org/EIPS/eip-6093
*/

// Indicates an error related to the current balance of a sender. Used in transfers.
error ERC20InsufficientBalance(address sender, uint256 balance, uint256 needed);
// Indicates a failure with the token sender. Used in transfers.
error ERC20InvalidSender(address sender);
// Indicates a failure with the token receiver. Used in transfers.
error ERC20InvalidReceiver(address receiver);
// Indicates a failure with the spender’s allowance. Used in transfers.
error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
// Indicates a failure with the approver of a token to be approved. Used in approvals.
error ERC20InvalidApprover(address approver);
// Indicates a failure with the spender to be approved. Used in approvals.
error ERC20InvalidSpender(address spender);

/** The following are custom implementations not present at EIP-6093 based on
* legacy requires.
*/

// Indicates an error related to reducing the allowance to less than zero
error ERC20InvalidAllowanceReduction(uint256 allowance, uint256 reduction);

/**
* @dev Implementation of the {IERC20} interface.
*
Expand Down Expand Up @@ -197,7 +227,15 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
function decreaseAllowance(address spender, uint256 subtractedValue) public virtual returns (bool) {
address owner = _msgSender();
uint256 currentAllowance = allowance(owner, spender);
require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero");

if (spender == address(0)) {
revert ERC20InvalidSpender(address(0));
}

if (currentAllowance < subtractedValue) {
revert ERC20InvalidAllowanceReduction(currentAllowance, subtractedValue);
}

unchecked {
_approve(owner, spender, currentAllowance - subtractedValue);
}
Expand All @@ -220,13 +258,22 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
* - `from` must have a balance of at least `amount`.
*/
function _transfer(address from, address to, uint256 amount) internal virtual {
require(from != address(0), "ERC20: transfer from the zero address");
require(to != address(0), "ERC20: transfer to the zero address");
if (from == address(0)) {
revert ERC20InvalidSender(from);
}

if (to == address(0)) {
revert ERC20InvalidReceiver(to);
}

_beforeTokenTransfer(from, to, amount);

uint256 fromBalance = _balances[from];
require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");

if (fromBalance < amount) {
revert ERC20InsufficientBalance(from, fromBalance, amount);
}

unchecked {
_balances[from] = fromBalance - amount;
// Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
Expand All @@ -249,7 +296,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
* - `account` cannot be the zero address.
*/
function _mint(address account, uint256 amount) internal virtual {
require(account != address(0), "ERC20: mint to the zero address");
if (account == address(0)) {
revert ERC20InvalidReceiver(account);
}

_beforeTokenTransfer(address(0), account, amount);

Expand All @@ -275,12 +324,18 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
* - `account` must have at least `amount` tokens.
*/
function _burn(address account, uint256 amount) internal virtual {
require(account != address(0), "ERC20: burn from the zero address");
if (account == address(0)) {
revert ERC20InvalidSender(account);
}

_beforeTokenTransfer(account, address(0), amount);

uint256 accountBalance = _balances[account];
require(accountBalance >= amount, "ERC20: burn amount exceeds balance");

if (accountBalance < amount) {
revert ERC20InsufficientBalance(account, accountBalance, amount);
}

unchecked {
_balances[account] = accountBalance - amount;
// Overflow not possible: amount <= accountBalance <= totalSupply.
Expand All @@ -306,8 +361,13 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
* - `spender` cannot be the zero address.
*/
function _approve(address owner, address spender, uint256 amount) internal virtual {
require(owner != address(0), "ERC20: approve from the zero address");
require(spender != address(0), "ERC20: approve to the zero address");
if (owner == address(0)) {
revert ERC20InvalidApprover(owner);
}

if (spender == address(0)) {
revert ERC20InvalidSpender(spender);
}

_allowances[owner][spender] = amount;
emit Approval(owner, spender, amount);
Expand All @@ -324,7 +384,10 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
function _spendAllowance(address owner, address spender, uint256 amount) internal virtual {
uint256 currentAllowance = allowance(owner, spender);
if (currentAllowance != type(uint256).max) {
require(currentAllowance >= amount, "ERC20: insufficient allowance");
if (currentAllowance < amount) {
revert ERC20InsufficientAllowance(spender, currentAllowance, amount);
}

unchecked {
_approve(owner, spender, currentAllowance - amount);
}
Expand Down
24 changes: 16 additions & 8 deletions test/token/ERC20/ERC20.behavior.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test
const { expect } = require('chai');
const { ZERO_ADDRESS, MAX_UINT256 } = constants;

const CUSTOM_ERROR = 'reverted with custom error ';

function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipient, anotherAccount) {
describe('total supply', function () {
it('returns the total amount of tokens', async function () {
Expand Down Expand Up @@ -87,7 +89,7 @@ function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipi
it('reverts', async function () {
await expectRevert(
this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
`${errorPrefix}: transfer amount exceeds balance`,
CUSTOM_ERROR + `'${errorPrefix}InsufficientBalance("${tokenOwner}", ${initialSupply - 1}, ${amount})'`,
);
});
});
Expand All @@ -106,7 +108,7 @@ function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipi
it('reverts', async function () {
await expectRevert(
this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
`${errorPrefix}: insufficient allowance`,
CUSTOM_ERROR + `'${errorPrefix}InsufficientAllowance("${spender}", ${initialSupply - 1}, ${amount})'`,
);
});
});
Expand All @@ -121,7 +123,7 @@ function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipi
it('reverts', async function () {
await expectRevert(
this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
`${errorPrefix}: transfer amount exceeds balance`,
CUSTOM_ERROR + `'${errorPrefix}InsufficientBalance("${tokenOwner}", ${initialSupply - 2}, ${amount})'`,
);
});
});
Expand Down Expand Up @@ -155,7 +157,7 @@ function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipi
it('reverts', async function () {
await expectRevert(
this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
`${errorPrefix}: transfer to the zero address`,
CUSTOM_ERROR + `'${errorPrefix}InvalidReceiver("${to}")'`,
);
});
});
Expand All @@ -167,7 +169,10 @@ function shouldBehaveLikeERC20(errorPrefix, initialSupply, initialHolder, recipi
const to = recipient;

it('reverts', async function () {
await expectRevert(this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'from the zero address');
await expectRevert(
this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
CUSTOM_ERROR + `'${errorPrefix}InvalidApprover("${tokenOwner}")'`,
);
});
});
});
Expand All @@ -191,7 +196,10 @@ function shouldBehaveLikeERC20Transfer(errorPrefix, from, to, balance, transfer)
const amount = balance.addn(1);

it('reverts', async function () {
await expectRevert(transfer.call(this, from, to, amount), `${errorPrefix}: transfer amount exceeds balance`);
await expectRevert(
transfer.call(this, from, to, amount),
CUSTOM_ERROR + `'${errorPrefix}InsufficientBalance("${from}", ${balance}, ${amount})'`,
);
});
});

Expand Down Expand Up @@ -232,7 +240,7 @@ function shouldBehaveLikeERC20Transfer(errorPrefix, from, to, balance, transfer)
it('reverts', async function () {
await expectRevert(
transfer.call(this, from, ZERO_ADDRESS, balance),
`${errorPrefix}: transfer to the zero address`,
CUSTOM_ERROR + `'${errorPrefix}InvalidReceiver("${ZERO_ADDRESS}")'`,
);
});
});
Expand Down Expand Up @@ -309,7 +317,7 @@ function shouldBehaveLikeERC20Approve(errorPrefix, owner, spender, supply, appro
it('reverts', async function () {
await expectRevert(
approve.call(this, owner, ZERO_ADDRESS, supply),
`${errorPrefix}: approve to the zero address`,
CUSTOM_ERROR + `'${errorPrefix}InvalidSpender("${ZERO_ADDRESS}")'`,
);
});
});
Expand Down
26 changes: 17 additions & 9 deletions test/token/ERC20/ERC20.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const {
const ERC20 = artifacts.require('$ERC20');
const ERC20Decimals = artifacts.require('$ERC20DecimalsMock');

const CUSTOM_ERROR = 'reverted with custom error ';

contract('ERC20', function (accounts) {
const [initialHolder, recipient, anotherAccount] = accounts;

Expand Down Expand Up @@ -56,7 +58,7 @@ contract('ERC20', function (accounts) {
it('reverts', async function () {
await expectRevert(
this.token.decreaseAllowance(spender, amount, { from: initialHolder }),
'ERC20: decreased allowance below zero',
CUSTOM_ERROR + `'ERC20InvalidAllowanceReduction(0, ${amount})'`,
);
});
});
Expand Down Expand Up @@ -90,7 +92,7 @@ contract('ERC20', function (accounts) {
it('reverts when more than the full allowance is removed', async function () {
await expectRevert(
this.token.decreaseAllowance(spender, approvedAmount.addn(1), { from: initialHolder }),
'ERC20: decreased allowance below zero',
CUSTOM_ERROR + `'ERC20InvalidAllowanceReduction(${approvedAmount}, ${approvedAmount.addn(1)})'`,
);
});
});
Expand All @@ -116,7 +118,7 @@ contract('ERC20', function (accounts) {
it('reverts', async function () {
await expectRevert(
this.token.decreaseAllowance(spender, amount, { from: initialHolder }),
'ERC20: decreased allowance below zero',
CUSTOM_ERROR + `'ERC20InvalidSpender("${spender}")'`,
);
});
});
Expand Down Expand Up @@ -197,7 +199,7 @@ contract('ERC20', function (accounts) {
it('reverts', async function () {
await expectRevert(
this.token.increaseAllowance(spender, amount, { from: initialHolder }),
'ERC20: approve to the zero address',
CUSTOM_ERROR + `'ERC20InvalidSpender("${spender}")'`,
);
});
});
Expand All @@ -206,7 +208,10 @@ contract('ERC20', function (accounts) {
describe('_mint', function () {
const amount = new BN(50);
it('rejects a null account', async function () {
await expectRevert(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20: mint to the zero address');
await expectRevert(
this.token.$_mint(ZERO_ADDRESS, amount),
CUSTOM_ERROR + `'ERC20InvalidReceiver("${ZERO_ADDRESS}")'`,
);
});

describe('for a non zero account', function () {
Expand All @@ -233,14 +238,17 @@ contract('ERC20', function (accounts) {

describe('_burn', function () {
it('rejects a null account', async function () {
await expectRevert(this.token.$_burn(ZERO_ADDRESS, new BN(1)), 'ERC20: burn from the zero address');
await expectRevert(
this.token.$_burn(ZERO_ADDRESS, new BN(1)),
CUSTOM_ERROR + `'ERC20InvalidSender("${ZERO_ADDRESS}")'`,
);
});

describe('for a non zero account', function () {
it('rejects burning more than balance', async function () {
await expectRevert(
this.token.$_burn(initialHolder, initialSupply.addn(1)),
'ERC20: burn amount exceeds balance',
CUSTOM_ERROR + `'ERC20InsufficientBalance("${initialHolder}", ${initialSupply}, ${initialSupply.addn(1)})'`,
);
});

Expand Down Expand Up @@ -282,7 +290,7 @@ contract('ERC20', function (accounts) {
it('reverts', async function () {
await expectRevert(
this.token.$_transfer(ZERO_ADDRESS, recipient, initialSupply),
'ERC20: transfer from the zero address',
CUSTOM_ERROR + `'ERC20InvalidSender("${ZERO_ADDRESS}")'`,
);
});
});
Expand All @@ -297,7 +305,7 @@ contract('ERC20', function (accounts) {
it('reverts', async function () {
await expectRevert(
this.token.$_approve(ZERO_ADDRESS, recipient, initialSupply),
'ERC20: approve from the zero address',
CUSTOM_ERROR + `'ERC20InvalidApprover("${ZERO_ADDRESS}")'`,
);
});
});
Expand Down
14 changes: 11 additions & 3 deletions test/token/ERC20/extensions/ERC20Burnable.behavior.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ const { ZERO_ADDRESS } = constants;

const { expect } = require('chai');

const CUSTOM_ERROR = 'reverted with custom error ';

function shouldBehaveLikeERC20Burnable(owner, initialBalance, [burner]) {
describe('burn', function () {
describe('when the given amount is not greater than balance of the sender', function () {
Expand Down Expand Up @@ -37,7 +39,10 @@ function shouldBehaveLikeERC20Burnable(owner, initialBalance, [burner]) {
const amount = initialBalance.addn(1);

it('reverts', async function () {
await expectRevert(this.token.burn(amount, { from: owner }), 'ERC20: burn amount exceeds balance');
await expectRevert(
this.token.burn(amount, { from: owner }),
CUSTOM_ERROR + `'ERC20InsufficientBalance("${owner}", ${initialBalance}, ${amount})'`,
);
});
});
});
Expand Down Expand Up @@ -83,7 +88,10 @@ function shouldBehaveLikeERC20Burnable(owner, initialBalance, [burner]) {

it('reverts', async function () {
await this.token.approve(burner, amount, { from: owner });
await expectRevert(this.token.burnFrom(owner, amount, { from: burner }), 'ERC20: burn amount exceeds balance');
await expectRevert(
this.token.burnFrom(owner, amount, { from: burner }),
CUSTOM_ERROR + `'ERC20InsufficientBalance("${owner}", ${initialBalance}, ${amount})'`,
);
});
});

Expand All @@ -94,7 +102,7 @@ function shouldBehaveLikeERC20Burnable(owner, initialBalance, [burner]) {
await this.token.approve(burner, allowance, { from: owner });
await expectRevert(
this.token.burnFrom(owner, allowance.addn(1), { from: burner }),
'ERC20: insufficient allowance',
CUSTOM_ERROR + `'ERC20InsufficientAllowance("${burner}", ${allowance}, ${allowance.addn(1)})'`,
);
});
});
Expand Down
6 changes: 4 additions & 2 deletions test/token/ERC20/extensions/ERC20FlashMint.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const { MAX_UINT256, ZERO_ADDRESS } = constants;
const ERC20FlashMintMock = artifacts.require('$ERC20FlashMintMock');
const ERC3156FlashBorrowerMock = artifacts.require('ERC3156FlashBorrowerMock');

const CUSTOM_ERROR = 'reverted with custom error ';

contract('ERC20FlashMint', function (accounts) {
const [initialHolder, other, anotherAccount] = accounts;

Expand Down Expand Up @@ -89,7 +91,7 @@ contract('ERC20FlashMint', function (accounts) {
const receiver = await ERC3156FlashBorrowerMock.new(true, false);
await expectRevert(
this.token.flashLoan(receiver.address, this.token.address, loanAmount, '0x'),
'ERC20: insufficient allowance',
CUSTOM_ERROR + `'ERC20InsufficientAllowance("${this.token.address}", 0, ${loanAmount})'`,
);
});

Expand All @@ -98,7 +100,7 @@ contract('ERC20FlashMint', function (accounts) {
const data = this.token.contract.methods.transfer(other, 10).encodeABI();
await expectRevert(
this.token.flashLoan(receiver.address, this.token.address, loanAmount, data),
'ERC20: burn amount exceeds balance',
CUSTOM_ERROR + `'ERC20InsufficientBalance("${receiver.address}", ${loanAmount.addn(-10)}, ${loanAmount})'`,
);
});

Expand Down
Loading

0 comments on commit bb8ba79

Please sign in to comment.