Remove non-standard increaseAllowance and decreaseAllowance from ERC20 (#4585)

Co-authored-by: Francisco <fg@frang.io>

.changeset/six-frogs-turn.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC20`: Remove the non-standard `increaseAllowance` and `decreaseAllowance` functions.

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                                                                              │

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`.
      *

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 () {