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

Jump to bottom

SIP-335 allow approve amount 0 to revoke erc20 #1742

Merged
merged 4 commits into from
Aug 16, 2023
Merged

SIP-335 allow approve amount 0 to revoke erc20 #1742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98d7c8b
SIP-335 allow approve amount 0 to revoke erc20
davidvuong Aug 3, 2023
58347f5
Merge branch 'main' into SIP-335
sunnyvempati Aug 8, 2023
5d41088
Reuse _checkZeroAddress in _checkZeroAddressOrAmount
davidvuong Aug 10, 2023
da6cbb1
Merge branch 'main' into SIP-335
davidvuong Aug 10, 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
8 changes: 7 additions & 1 deletion utils/core-contracts/contracts/token/ERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ contract ERC20 is IERC20 {
}

function _approve(address owner, address spender, uint256 amount) internal virtual {
_checkZeroAddressOrAmount(spender, amount);
_checkZeroAddress(spender);

ERC20Storage.load().allowance[owner][spender] = amount;

Expand All @@ -175,6 +175,12 @@ contract ERC20 is IERC20 {
}
}

function _checkZeroAddress(address target) private pure {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe reuse this by calling this function in _checkZeroAddressOrAmount. nonblocking

if (target == address(0)) {
revert ParameterError.InvalidParameter("target", "Zero address");
}
}

function _mint(address to, uint256 amount) internal virtual {
_checkZeroAddressOrAmount(to, amount);

Expand Down
18 changes: 14 additions & 4 deletions utils/core-contracts/test/contracts/token/ERC20.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@ describe('ERC20', function () {
);

// Zero amount
await assertRevert(
ERC20.approve(await user1.getAddress(), 0),
'InvalidParameter("amount", "Zero amount")'
);
await assertRevert(
ERC20.transfer(await user1.getAddress(), 0),
'InvalidParameter("amount", "Zero amount")'
Expand Down Expand Up @@ -221,6 +217,20 @@ describe('ERC20', function () {
assertBn.equal(evt.args.amount, approvalAmount);
});

describe('approve()', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming there's already a test for zero address?

it('revokes token when amount zero', async function () {
await ERC20.connect(user1).approve(await user2.getAddress(), BigNumber.from('0'));
assertBn.equal(
await ERC20.allowance(await user1.getAddress(), await user2.getAddress()),
'0'
);
});

after('reset approval', async () => {
await ERC20.connect(user1).approve(await user2.getAddress(), approvalAmount);
});
});

describe('increaseAllowance()', async () => {
it('reverts when overflowing', async () => {
await assertRevert(
Expand Down
Loading