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.

Sign up for GitHub

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

Remove non-standard increaseAllowance and decreaseAllowance from ERC20 #4585

Merged
merged 4 commits into from
Sep 12, 2023
Merged

Remove non-standard increaseAllowance and decreaseAllowance from ERC20 #4585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f86ea3
remove ERC20.increaseAllowance and ERC20.decreaseAllowance
Amxx Sep 7, 2023
3723e39
add changeset
Amxx Sep 7, 2023
dfe1a7d
Update six-frogs-turn.md
frangio Sep 7, 2023
804ec79
Merge branch 'master' into refactor/ERC20/remove-increasse-decreasse-…
Amxx Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/six-frogs-turn.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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