Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functions in SafeMath contract overloaded to accept custom error messages #1828

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
49 changes: 46 additions & 3 deletions contracts/math/SafeMath.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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

Expand All @@ -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;
}
}
10 changes: 5 additions & 5 deletions contracts/token/ERC20/ERC20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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"));
}
}
2 changes: 1 addition & 1 deletion contracts/token/ERC20/SafeERC20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
6 changes: 3 additions & 3 deletions contracts/token/ERC777/ERC777.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
8 changes: 4 additions & 4 deletions test/token/ERC20/ERC20.behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
);
});
});
Expand All @@ -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`
);
});
});
Expand All @@ -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`
);
});
});
Expand Down Expand Up @@ -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`
);
});
});
Expand Down
12 changes: 6 additions & 6 deletions test/token/ERC20/ERC20.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
});
});
Expand Down Expand Up @@ -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'
);
});
});
Expand All @@ -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'
);
});
});
Expand Down Expand Up @@ -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'
);
});

Expand Down Expand Up @@ -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'
);
});

Expand Down
4 changes: 2 additions & 2 deletions test/token/ERC20/SafeERC20.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
});
});
Expand Down Expand Up @@ -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'
);
});
});
Expand Down
6 changes: 3 additions & 3 deletions test/token/ERC20/behaviors/ERC20Burnable.behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
});
});
Expand Down Expand Up @@ -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'
);
});
});
Expand All @@ -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'
);
});
});
Expand Down