From 60e3ffe6a3cc38ab94cae995bc1de081eed79335 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 12 Sep 2023 16:59:48 +0200 Subject: [PATCH] Remove non-standard increaseAllowance and decreaseAllowance from ERC20 (#4585) Co-authored-by: Francisco --- .changeset/six-frogs-turn.md | 5 + certora/specs/ERC20.spec | 72 -------------- contracts/token/ERC20/ERC20.sol | 52 ----------- test/token/ERC20/ERC20.test.js | 161 -------------------------------- 4 files changed, 5 insertions(+), 285 deletions(-) create mode 100644 .changeset/six-frogs-turn.md diff --git a/.changeset/six-frogs-turn.md b/.changeset/six-frogs-turn.md new file mode 100644 index 00000000000..9c5668b6db5 --- /dev/null +++ b/.changeset/six-frogs-turn.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC20`: Remove the non-standard `increaseAllowance` and `decreaseAllowance` functions. diff --git a/certora/specs/ERC20.spec b/certora/specs/ERC20.spec index ee601bd19da..21a03358508 100644 --- a/certora/specs/ERC20.spec +++ b/certora/specs/ERC20.spec @@ -3,10 +3,6 @@ import "methods/IERC20.spec"; import "methods/IERC2612.spec"; methods { - // non standard ERC20 functions - function increaseAllowance(address,uint256) external returns (bool); - function decreaseAllowance(address,uint256) external returns (bool); - // exposed for FV function mint(address,uint256) external; function burn(address,uint256) external; @@ -117,7 +113,6 @@ rule onlyHolderOfSpenderCanChangeAllowance(env e) { allowanceAfter > allowanceBefore ) => ( (f.selector == sig:approve(address,uint256).selector && e.msg.sender == holder) || - (f.selector == sig:increaseAllowance(address,uint256).selector && e.msg.sender == holder) || (f.selector == sig:permit(address,address,uint256,uint256,uint8,bytes32,bytes32).selector) ); @@ -126,7 +121,6 @@ rule onlyHolderOfSpenderCanChangeAllowance(env e) { ) => ( (f.selector == sig:transferFrom(address,address,uint256).selector && e.msg.sender == spender) || (f.selector == sig:approve(address,uint256).selector && e.msg.sender == holder ) || - (f.selector == sig:decreaseAllowance(address,uint256).selector && e.msg.sender == holder ) || (f.selector == sig:permit(address,address,uint256,uint256,uint8,bytes32,bytes32).selector) ); } @@ -307,72 +301,6 @@ rule approve(env e) { } } -/* -┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Rule: increaseAllowance behavior and side effects │ -└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ -*/ -rule increaseAllowance(env e) { - require nonpayable(e); - - address holder = e.msg.sender; - address spender; - address otherHolder; - address otherSpender; - uint256 amount; - - // cache state - uint256 allowanceBefore = allowance(holder, spender); - uint256 otherAllowanceBefore = allowance(otherHolder, otherSpender); - - // run transaction - increaseAllowance@withrevert(e, spender, amount); - - // check outcome - if (lastReverted) { - assert holder == 0 || spender == 0 || allowanceBefore + amount > max_uint256; - } else { - // allowance is updated - assert to_mathint(allowance(holder, spender)) == allowanceBefore + amount; - - // other allowances are untouched - assert allowance(otherHolder, otherSpender) != otherAllowanceBefore => (otherHolder == holder && otherSpender == spender); - } -} - -/* -┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Rule: decreaseAllowance behavior and side effects │ -└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ -*/ -rule decreaseAllowance(env e) { - require nonpayable(e); - - address holder = e.msg.sender; - address spender; - address otherHolder; - address otherSpender; - uint256 amount; - - // cache state - uint256 allowanceBefore = allowance(holder, spender); - uint256 otherAllowanceBefore = allowance(otherHolder, otherSpender); - - // run transaction - decreaseAllowance@withrevert(e, spender, amount); - - // check outcome - if (lastReverted) { - assert holder == 0 || spender == 0 || allowanceBefore < amount; - } else { - // allowance is updated - assert to_mathint(allowance(holder, spender)) == allowanceBefore - amount; - - // other allowances are untouched - assert allowance(otherHolder, otherSpender) != otherAllowanceBefore => (otherHolder == holder && otherSpender == spender); - } -} - /* ┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ Rule: permit behavior and side effects │ diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 8eeb3149b06..692fde82827 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -30,10 +30,6 @@ import {IERC20Errors} from "../../interfaces/draft-IERC6093.sol"; * This allows applications to reconstruct the allowance for all accounts just * by listening to said events. Other implementations of the EIP may not emit * these events, as it isn't required by the specification. - * - * Finally, the non-standard {decreaseAllowance} and {increaseAllowance} - * functions have been added to mitigate the well-known issues around setting - * allowances. See {IERC20-approve}. */ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { mapping(address account => uint256) private _balances; @@ -167,54 +163,6 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { return true; } - /** - * @dev Atomically increases the allowance granted to `spender` by the caller. - * - * This is an alternative to {approve} that can be used as a mitigation for - * problems described in {IERC20-approve}. - * - * Emits an {Approval} event indicating the updated allowance. - * - * Requirements: - * - * - `spender` cannot be the zero address. - */ - function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) { - address owner = _msgSender(); - _approve(owner, spender, allowance(owner, spender) + addedValue); - return true; - } - - /** - * @dev Atomically decreases the allowance granted to `spender` by the caller. - * - * This is an alternative to {approve} that can be used as a mitigation for - * problems described in {IERC20-approve}. - * - * Emits an {Approval} event indicating the updated allowance. - * - * Requirements: - * - * - `spender` cannot be the zero address. - * - `spender` must have allowance for the caller of at least - * `requestedDecrease`. - * - * NOTE: Although this function is designed to avoid double spending with {approval}, - * it can still be frontrunned, preventing any attempt of allowance reduction. - */ - function decreaseAllowance(address spender, uint256 requestedDecrease) public virtual returns (bool) { - address owner = _msgSender(); - uint256 currentAllowance = allowance(owner, spender); - if (currentAllowance < requestedDecrease) { - revert ERC20FailedDecreaseAllowance(spender, currentAllowance, requestedDecrease); - } - unchecked { - _approve(owner, spender, currentAllowance - requestedDecrease); - } - - return true; - } - /** * @dev Moves a `value` amount of tokens from `from` to `to`. * diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index a63df523958..2191fd8cb57 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -42,167 +42,6 @@ contract('ERC20', function (accounts) { expect(await this.token.decimals()).to.be.bignumber.equal('18'); }); - describe('decrease allowance', function () { - describe('when the spender is not the zero address', function () { - const spender = recipient; - - function shouldDecreaseApproval(value) { - describe('when there was no approved value before', function () { - it('reverts', async function () { - const allowance = await this.token.allowance(initialHolder, spender); - await expectRevertCustomError( - this.token.decreaseAllowance(spender, value, { from: initialHolder }), - 'ERC20FailedDecreaseAllowance', - [spender, allowance, value], - ); - }); - }); - - describe('when the spender had an approved value', function () { - const approvedValue = value; - - beforeEach(async function () { - await this.token.approve(spender, approvedValue, { from: initialHolder }); - }); - - it('emits an approval event', async function () { - expectEvent( - await this.token.decreaseAllowance(spender, approvedValue, { from: initialHolder }), - 'Approval', - { owner: initialHolder, spender: spender, value: new BN(0) }, - ); - }); - - it('decreases the spender allowance subtracting the requested value', async function () { - await this.token.decreaseAllowance(spender, approvedValue.subn(1), { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('1'); - }); - - it('sets the allowance to zero when all allowance is removed', async function () { - await this.token.decreaseAllowance(spender, approvedValue, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('0'); - }); - - it('reverts when more than the full allowance is removed', async function () { - await expectRevertCustomError( - this.token.decreaseAllowance(spender, approvedValue.addn(1), { from: initialHolder }), - 'ERC20FailedDecreaseAllowance', - [spender, approvedValue, approvedValue.addn(1)], - ); - }); - }); - } - - describe('when the sender has enough balance', function () { - const value = initialSupply; - - shouldDecreaseApproval(value); - }); - - describe('when the sender does not have enough balance', function () { - const value = initialSupply.addn(1); - - shouldDecreaseApproval(value); - }); - }); - - describe('when the spender is the zero address', function () { - const value = initialSupply; - const spender = ZERO_ADDRESS; - - it('reverts', async function () { - await expectRevertCustomError( - this.token.decreaseAllowance(spender, value, { from: initialHolder }), - 'ERC20FailedDecreaseAllowance', - [spender, 0, value], - ); - }); - }); - }); - - describe('increase allowance', function () { - const value = initialSupply; - - describe('when the spender is not the zero address', function () { - const spender = recipient; - - describe('when the sender has enough balance', function () { - it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, value, { from: initialHolder }), 'Approval', { - owner: initialHolder, - spender: spender, - value: value, - }); - }); - - describe('when there was no approved value before', function () { - it('approves the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value); - }); - }); - - describe('when the spender had an approved value', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); - }); - - it('increases the spender allowance adding the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value.addn(1)); - }); - }); - }); - - describe('when the sender does not have enough balance', function () { - const value = initialSupply.addn(1); - - it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, value, { from: initialHolder }), 'Approval', { - owner: initialHolder, - spender: spender, - value: value, - }); - }); - - describe('when there was no approved value before', function () { - it('approves the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value); - }); - }); - - describe('when the spender had an approved value', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); - }); - - it('increases the spender allowance adding the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value.addn(1)); - }); - }); - }); - }); - - describe('when the spender is the zero address', function () { - const spender = ZERO_ADDRESS; - - it('reverts', async function () { - await expectRevertCustomError( - this.token.increaseAllowance(spender, value, { from: initialHolder }), - 'ERC20InvalidSpender', - [ZERO_ADDRESS], - ); - }); - }); - }); - describe('_mint', function () { const value = new BN(50); it('rejects a null account', async function () {