diff --git a/CHANGELOG.md b/CHANGELOG.md index 71e201f6a74..3e8944137f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Improvements: * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802)) + * `SafeMath`: added custom error messages support for `sub`, `div` and `mod` functions. `ERC20` and `ERC777` updated to throw custom errors on subtraction overflows. ([#1828](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1828)) ### Bugfixes diff --git a/contracts/math/SafeMath.sol b/contracts/math/SafeMath.sol index 2f1192f00d4..5cb05fafdf0 100644 --- a/contracts/math/SafeMath.sol +++ b/contracts/math/SafeMath.sol @@ -40,7 +40,20 @@ library SafeMath { * - Subtraction cannot overflow. */ function sub(uint256 a, uint256 b) internal pure returns (uint256) { - require(b <= a, "SafeMath: subtraction overflow"); + return sub(a, b, "SafeMath: subtraction overflow"); + } + + /** + * @dev Returns the subtraction of two unsigned integers, reverting with custom message on + * overflow (when the result is negative). + * + * Counterpart to Solidity's `-` operator. + * + * Requirements: + * - Subtraction cannot overflow. + */ + function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) { + require(b <= a, errorMessage); uint256 c = a - b; return c; @@ -81,8 +94,23 @@ library SafeMath { * - The divisor cannot be zero. */ function div(uint256 a, uint256 b) internal pure returns (uint256) { + return div(a, b, "SafeMath: division by zero"); + } + + /** + * @dev Returns the integer division of two unsigned integers. Reverts with custom message on + * division by zero. The result is rounded towards zero. + * + * Counterpart to Solidity's `/` operator. Note: this function uses a + * `revert` opcode (which leaves remaining gas untouched) while Solidity + * uses an invalid opcode to revert (consuming all remaining gas). + * + * Requirements: + * - The divisor cannot be zero. + */ + function div(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) { // Solidity only automatically asserts when dividing by 0 - require(b > 0, "SafeMath: division by zero"); + require(b > 0, errorMessage); uint256 c = a / b; // assert(a == b * c + a % b); // There is no case in which this doesn't hold @@ -101,7 +129,22 @@ library SafeMath { * - The divisor cannot be zero. */ function mod(uint256 a, uint256 b) internal pure returns (uint256) { - require(b != 0, "SafeMath: modulo by zero"); + return mod(a, b, "SafeMath: modulo by zero"); + } + + /** + * @dev Returns the remainder of dividing two unsigned integers. (unsigned integer modulo), + * Reverts with custom message when dividing by zero. + * + * Counterpart to Solidity's `%` operator. This function uses a `revert` + * opcode (which leaves remaining gas untouched) while Solidity uses an + * invalid opcode to revert (consuming all remaining gas). + * + * Requirements: + * - The divisor cannot be zero. + */ + function mod(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) { + require(b != 0, errorMessage); return a % b; } } diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 0323f899161..5f4de37582a 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -96,7 +96,7 @@ contract ERC20 is IERC20 { */ function transferFrom(address sender, address recipient, uint256 amount) public returns (bool) { _transfer(sender, recipient, amount); - _approve(sender, msg.sender, _allowances[sender][msg.sender].sub(amount)); + _approve(sender, msg.sender, _allowances[sender][msg.sender].sub(amount, "ERC20: transfer amount exceeds allowance")); return true; } @@ -132,7 +132,7 @@ contract ERC20 is IERC20 { * `subtractedValue`. */ function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) { - _approve(msg.sender, spender, _allowances[msg.sender][spender].sub(subtractedValue)); + _approve(msg.sender, spender, _allowances[msg.sender][spender].sub(subtractedValue, "ERC20: decreased allowance below zero")); return true; } @@ -154,7 +154,7 @@ contract ERC20 is IERC20 { require(sender != address(0), "ERC20: transfer from the zero address"); require(recipient != address(0), "ERC20: transfer to the zero address"); - _balances[sender] = _balances[sender].sub(amount); + _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance"); _balances[recipient] = _balances[recipient].add(amount); emit Transfer(sender, recipient, amount); } @@ -190,8 +190,8 @@ contract ERC20 is IERC20 { function _burn(address account, uint256 value) internal { require(account != address(0), "ERC20: burn from the zero address"); + _balances[account] = _balances[account].sub(value, "ERC20: burn amount exceeds balance"); _totalSupply = _totalSupply.sub(value); - _balances[account] = _balances[account].sub(value); emit Transfer(account, address(0), value); } @@ -224,6 +224,6 @@ contract ERC20 is IERC20 { */ function _burnFrom(address account, uint256 amount) internal { _burn(account, amount); - _approve(account, msg.sender, _allowances[account][msg.sender].sub(amount)); + _approve(account, msg.sender, _allowances[account][msg.sender].sub(amount, "ERC20: burn amount exceeds allowance")); } } diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol index 63378b71b26..8cbc7b2f74d 100644 --- a/contracts/token/ERC20/SafeERC20.sol +++ b/contracts/token/ERC20/SafeERC20.sol @@ -42,7 +42,7 @@ library SafeERC20 { } function safeDecreaseAllowance(IERC20 token, address spender, uint256 value) internal { - uint256 newAllowance = token.allowance(address(this), spender).sub(value); + uint256 newAllowance = token.allowance(address(this), spender).sub(value, "SafeERC20: decreased allowance below zero"); callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance)); } diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 6aacd88ba86..feb511b8f17 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -285,7 +285,7 @@ contract ERC777 is IERC777, IERC20 { _callTokensToSend(spender, holder, recipient, amount, "", ""); _move(spender, holder, recipient, amount, "", ""); - _approve(holder, spender, _allowances[holder][spender].sub(amount)); + _approve(holder, spender, _allowances[holder][spender].sub(amount, "ERC777: transfer amount exceeds allowance")); _callTokensReceived(spender, holder, recipient, amount, "", "", false); @@ -383,8 +383,8 @@ contract ERC777 is IERC777, IERC20 { _callTokensToSend(operator, from, address(0), amount, data, operatorData); // Update state variables + _balances[from] = _balances[from].sub(amount, "ERC777: burn amount exceeds balance"); _totalSupply = _totalSupply.sub(amount); - _balances[from] = _balances[from].sub(amount); emit Burned(operator, from, amount, data, operatorData); emit Transfer(from, address(0), amount); @@ -400,7 +400,7 @@ contract ERC777 is IERC777, IERC20 { ) private { - _balances[from] = _balances[from].sub(amount); + _balances[from] = _balances[from].sub(amount, "ERC777: transfer amount exceeds balance"); _balances[to] = _balances[to].add(amount); emit Sent(operator, from, to, amount, userData, operatorData); diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 662e0f30b22..161664cc95a 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -88,7 +88,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds balance` ); }); }); @@ -104,7 +104,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds allowance` ); }); }); @@ -114,7 +114,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds balance` ); }); }); @@ -166,7 +166,7 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer it('reverts', async function () { await expectRevert(transfer.call(this, from, to, amount), - 'SafeMath: subtraction overflow' + `${errorPrefix}: transfer amount exceeds balance` ); }); }); diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index a04bb8c3ac6..3d65f922a56 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -27,7 +27,7 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { describe('when there was no approved amount before', function () { it('reverts', async function () { await expectRevert(this.token.decreaseAllowance( - spender, amount, { from: initialHolder }), 'SafeMath: subtraction overflow' + spender, amount, { from: initialHolder }), 'ERC20: decreased allowance below zero' ); }); }); @@ -63,7 +63,7 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { it('reverts when more than the full allowance is removed', async function () { await expectRevert( this.token.decreaseAllowance(spender, approvedAmount.addn(1), { from: initialHolder }), - 'SafeMath: subtraction overflow' + 'ERC20: decreased allowance below zero' ); }); }); @@ -88,7 +88,7 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { it('reverts', async function () { await expectRevert(this.token.decreaseAllowance( - spender, amount, { from: initialHolder }), 'SafeMath: subtraction overflow' + spender, amount, { from: initialHolder }), 'ERC20: decreased allowance below zero' ); }); }); @@ -221,7 +221,7 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { describe('for a non zero account', function () { it('rejects burning more than balance', async function () { await expectRevert(this.token.burn( - initialHolder, initialSupply.addn(1)), 'SafeMath: subtraction overflow' + initialHolder, initialSupply.addn(1)), 'ERC20: burn amount exceeds balance' ); }); @@ -276,13 +276,13 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { describe('for a non zero account', function () { it('rejects burning more than allowance', async function () { await expectRevert(this.token.burnFrom(initialHolder, allowance.addn(1)), - 'SafeMath: subtraction overflow' + 'ERC20: burn amount exceeds allowance' ); }); it('rejects burning more than balance', async function () { await expectRevert(this.token.burnFrom(initialHolder, initialSupply.addn(1)), - 'SafeMath: subtraction overflow' + 'ERC20: burn amount exceeds balance' ); }); diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js index 96ddb5fe40f..ec38b9fae48 100644 --- a/test/token/ERC20/SafeERC20.test.js +++ b/test/token/ERC20/SafeERC20.test.js @@ -93,7 +93,7 @@ function shouldOnlyRevertOnErrors () { it('reverts when decreasing the allowance', async function () { await expectRevert( this.wrapper.decreaseAllowance(10), - 'SafeMath: subtraction overflow' + 'SafeERC20: decreased allowance below zero' ); }); }); @@ -125,7 +125,7 @@ function shouldOnlyRevertOnErrors () { it('reverts when decreasing the allowance to a negative value', async function () { await expectRevert( this.wrapper.decreaseAllowance(200), - 'SafeMath: subtraction overflow' + 'SafeERC20: decreased allowance below zero' ); }); }); diff --git a/test/token/ERC20/behaviors/ERC20Burnable.behavior.js b/test/token/ERC20/behaviors/ERC20Burnable.behavior.js index b770f855ab0..9c43c366bb8 100644 --- a/test/token/ERC20/behaviors/ERC20Burnable.behavior.js +++ b/test/token/ERC20/behaviors/ERC20Burnable.behavior.js @@ -38,7 +38,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) { it('reverts', async function () { await expectRevert(this.token.burn(amount, { from: owner }), - 'SafeMath: subtraction overflow' + 'ERC20: burn amount exceeds balance' ); }); }); @@ -87,7 +87,7 @@ 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 }), - 'SafeMath: subtraction overflow' + 'ERC20: burn amount exceeds balance' ); }); }); @@ -98,7 +98,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) { it('reverts', async function () { await this.token.approve(burner, allowance, { from: owner }); await expectRevert(this.token.burnFrom(owner, allowance.addn(1), { from: burner }), - 'SafeMath: subtraction overflow' + 'ERC20: burn amount exceeds allowance' ); }); });