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

Do not reduce approval on transferFrom if current allowance is type(uint256).max #3085

Merged
merged 13 commits into from
Jan 10, 2022
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
* `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917))
* `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs`. ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884))
* `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056))
* `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085))
* `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085))

## 4.4.1 (2021-12-14)

Expand Down
14 changes: 11 additions & 3 deletions contracts/token/ERC20/ERC20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
/**
* @dev See {IERC20-approve}.
*
* NOTE: If `amount` is the maximum `uint256`, the allowance is not updated on
* `transferFrom`. This is semantically equivalent to an infinite approval.
*
* Requirements:
*
* - `spender` cannot be the zero address.
Expand All @@ -140,6 +143,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
* Emits an {Approval} event indicating the updated allowance. This is not
* required by the EIP. See the note at the beginning of {ERC20}.
*
* NOTE: Does not update the allowance if the current allowance
* is the maximum `uint256`.
*
* Requirements:
*
* - `sender` and `recipient` cannot be the zero address.
Expand All @@ -153,9 +159,11 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
uint256 amount
) public virtual override returns (bool) {
uint256 currentAllowance = _allowances[sender][_msgSender()];
require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
unchecked {
_approve(sender, _msgSender(), currentAllowance - amount);
if (currentAllowance != type(uint256).max) {
require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
unchecked {
_approve(sender, _msgSender(), currentAllowance - amount);
}
}

_transfer(sender, recipient, amount);
Expand Down
14 changes: 12 additions & 2 deletions contracts/token/ERC777/ERC777.sol
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,9 @@ contract ERC777 is Context, IERC777, IERC20 {
/**
* @dev See {IERC20-approve}.
*
* NOTE: If `value` is the maximum `uint256`, the allowance is not updated on
* `transferFrom`. This is semantically equivalent to an infinite approval.
*
* Note that accounts cannot have allowance issued by their operators.
*/
function approve(address spender, uint256 value) public virtual override returns (bool) {
Expand All @@ -269,6 +272,9 @@ contract ERC777 is Context, IERC777, IERC20 {
/**
* @dev See {IERC20-transferFrom}.
*
* NOTE: Does not update the allowance if the current allowance
* is the maximum `uint256`.
*
* Note that operator and allowance concepts are orthogonal: operators cannot
* call `transferFrom` (unless they have allowance), and accounts with
* allowance cannot call `operatorSend` (unless they are operators).
Expand All @@ -288,8 +294,12 @@ contract ERC777 is Context, IERC777, IERC20 {
_callTokensToSend(spender, holder, recipient, amount, "", "");

uint256 currentAllowance = _allowances[holder][spender];
require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance");
_approve(holder, spender, currentAllowance - amount);
if (currentAllowance != type(uint256).max) {
require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance");
unchecked {
_approve(holder, spender, currentAllowance - amount);
}
}

_move(spender, holder, recipient, amount, "", "");

Expand Down
21 changes: 20 additions & 1 deletion test/token/ERC20/ERC20.behavior.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');
const { ZERO_ADDRESS } = constants;
const { ZERO_ADDRESS, MAX_UINT256 } = constants;

function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recipient, anotherAccount) {
describe('total supply', function () {
Expand Down Expand Up @@ -128,6 +128,25 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip
});
});
});

describe('when the spender has unlimited allowance', function () {
beforeEach(async function () {
await this.token.approve(spender, MAX_UINT256, { from: initialHolder });
});

it('does not decrease the spender allowance', async function () {
await this.token.transferFrom(tokenOwner, to, 1, { from: spender });

expect(await this.token.allowance(tokenOwner, spender)).to.be.bignumber.equal(MAX_UINT256);
});

it('does not emit an approval event', async function () {
expectEvent.notEmitted(
await this.token.transferFrom(tokenOwner, to, 1, { from: spender }),
'Approval',
);
});
});
});

describe('when the recipient is the zero address', function () {
Expand Down