Skip to content

Commit

Permalink
Remove non-standard increaseAllowance and decreaseAllowance from ERC20 (
Browse files Browse the repository at this point in the history 
#4585)

Co-authored-by: Francisco <[email protected]>
  • Loading branch information
@Amxx @frangio
Amxx and frangio authored Sep 12, 2023
1 parent 36bf1e4 commit 60e3ffe
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 285 deletions.
5 changes: 5 additions & 0 deletions .changeset/six-frogs-turn.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`ERC20`: Remove the non-standard `increaseAllowance` and `decreaseAllowance` functions.
72 changes: 0 additions & 72 deletions certora/specs/ERC20.spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
);

Expand All @@ -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)
);
}
Expand Down Expand Up @@ -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 │
Expand Down
52 changes: 0 additions & 52 deletions contracts/token/ERC20/ERC20.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`.
*
Expand Down
161 changes: 0 additions & 161 deletions test/token/ERC20/ERC20.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down

0 comments on commit 60e3ffe

Please sign in to comment.