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.

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

Add a SafeERC20:safePermit function #3280

Merged
merged 12 commits into from Jun 7, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,7 @@
* `CrossChainEnabledPolygonChild`: replace the `require` statement with the custom error `NotCrossChainCall`. ([#3380](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3380))
* `ERC20FlashMint`: Add customizable flash fee receiver. ([#3327](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3327))
* `ERC20TokenizedVault`: add an extension of `ERC20` that implements the ERC4626 Tokenized Vault Standard. ([#3171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3171))
* `SafeERC20`: add `safePermit`. ([#3280](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3280))
Amxx marked this conversation as resolved.
Show resolved Hide resolved
* `Math`: add a `mulDiv` function that can round the result either up or down. ([#3171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3171))
* `Strings`: add a new overloaded function `toHexString` that converts an `address` with fixed length of 20 bytes to its not checksummed ASCII `string` hexadecimal representation. ([#3403](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3403))
* `EnumerableMap`: add new `UintToUintMap` map type. ([#3338](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3338))
Expand Down
56 changes: 53 additions & 3 deletions contracts/mocks/SafeERC20Helper.sol
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.0;

import "../utils/Context.sol";
import "../token/ERC20/IERC20.sol";
import "../token/ERC20/extensions/draft-ERC20Permit.sol";
import "../token/ERC20/utils/SafeERC20.sol";

contract ERC20ReturnFalseMock is Context {
Expand Down Expand Up @@ -105,12 +106,49 @@ contract ERC20NoReturnMock is Context {
}
}

contract ERC20PermitNoRevertMock is
ERC20("ERC20PermitNoRevertMock", "ERC20PermitNoRevertMock"),
ERC20Permit("ERC20PermitNoRevertMock")
{
function getChainId() external view returns (uint256) {
return block.chainid;
}

function permitRevert(
Amxx marked this conversation as resolved.
Show resolved Hide resolved
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) public virtual {
super.permit(owner, spender, value, deadline, v, r, s);
}

function permit(
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) public virtual override {
try this.permitRevert(owner, spender, value, deadline, v, r, s) {
// do nothing
} catch {
// do nothing
}
}
}

contract SafeERC20Wrapper is Context {
using SafeERC20 for IERC20;
using SafeERC20 for IERC20Permit;

IERC20 private _token;
IERC20Permit private _token;

constructor(IERC20 token) {
constructor(IERC20Permit token) {
_token = token;
}

Expand All @@ -134,6 +172,18 @@ contract SafeERC20Wrapper is Context {
_token.safeDecreaseAllowance(address(0), amount);
}

function permit(
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) public {
_token.safePermit(owner, spender, value, deadline, v, r, s);
}

function setAllowance(uint256 allowance_) public {
ERC20ReturnTrueMock(address(_token)).setAllowance(allowance_);
}
Expand Down
4 changes: 3 additions & 1 deletion contracts/token/ERC20/extensions/draft-IERC20Permit.sol
Expand Up @@ -3,6 +3,8 @@

pragma solidity ^0.8.0;

import "../IERC20.sol";

/**
* @dev Interface of the ERC20 Permit extension allowing approvals to be made via signatures, as defined in
* https://eips.ethereum.org/EIPS/eip-2612[EIP-2612].
Expand All @@ -11,7 +13,7 @@ pragma solidity ^0.8.0;
* presenting a message signed by the account. By not relying on {IERC20-approve}, the token holder account doesn't
* need to send a transaction, and thus is not required to hold Ether at all.
*/
interface IERC20Permit {
interface IERC20Permit is IERC20 {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
/**
* @dev Sets `value` as the allowance of `spender` over ``owner``'s tokens,
* given ``owner``'s signed approval.
Expand Down
20 changes: 20 additions & 0 deletions contracts/token/ERC20/utils/SafeERC20.sol
Expand Up @@ -4,6 +4,7 @@
pragma solidity ^0.8.0;

import "../IERC20.sol";
import "../extensions/draft-IERC20Permit.sol";
import "../../../utils/Address.sol";

/**
Expand Down Expand Up @@ -79,6 +80,25 @@ library SafeERC20 {
}
}

function safePermit(
IERC20Permit token,
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) internal {
uint256 nonceBefore = token.nonces(owner);
_callOptionalReturn(
Amxx marked this conversation as resolved.
Show resolved Hide resolved
token,
abi.encodeWithSelector(token.permit.selector, owner, spender, value, deadline, v, r, s)
);
uint256 nonceAfter = token.nonces(owner);
require(nonceAfter == nonceBefore + 1, "SafeERC20: permit did not succeed");
}

/**
* @dev Imitates a Solidity high-level call (i.e. a regular function call to a contract), relaxing the requirement
* on the return value: the return value is optional (but if data is returned, it must not be false).
Expand Down
131 changes: 130 additions & 1 deletion test/token/ERC20/utils/SafeERC20.test.js
@@ -1,10 +1,17 @@
const { expectRevert } = require('@openzeppelin/test-helpers');
const { constants, expectRevert } = require('@openzeppelin/test-helpers');

const ERC20ReturnFalseMock = artifacts.require('ERC20ReturnFalseMock');
const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock');
const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock');
const ERC20PermitNoRevertMock = artifacts.require('ERC20PermitNoRevertMock');
const SafeERC20Wrapper = artifacts.require('SafeERC20Wrapper');

const { EIP712Domain } = require('../../../helpers/eip712');

const { fromRpcSig } = require('ethereumjs-util');
const ethSigUtil = require('eth-sig-util');
const Wallet = require('ethereumjs-wallet').default;

contract('SafeERC20', function (accounts) {
const [ hasNoCode ] = accounts;

Expand Down Expand Up @@ -39,6 +46,128 @@ contract('SafeERC20', function (accounts) {

shouldOnlyRevertOnErrors();
});

describe('with token that doesn\'t revert on invalid permit', function () {
const Permit = [
{ name: 'owner', type: 'address' },
{ name: 'spender', type: 'address' },
{ name: 'value', type: 'uint256' },
{ name: 'nonce', type: 'uint256' },
{ name: 'deadline', type: 'uint256' },
];

const wallet = Wallet.generate();
const owner = wallet.getAddressString();
const spender = hasNoCode;

beforeEach(async function () {
this.token = await ERC20PermitNoRevertMock.new();
this.wrapper = await SafeERC20Wrapper.new(this.token.address);

const chainId = await this.token.getChainId();

this.data = {
primaryType: 'Permit',
types: { EIP712Domain, Permit },
domain: { name: 'ERC20PermitNoRevertMock', version: '1', chainId, verifyingContract: this.token.address },
message: { owner, spender, value: '42', nonce: '0', deadline: constants.MAX_UINT256 },
};
this.signature = fromRpcSig(ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data: this.data }));
});

it('accepts owner signature', async function () {
expect(await this.token.nonces(owner)).to.be.bignumber.equal('0');
expect(await this.token.allowance(owner, spender)).to.be.bignumber.equal('0');

await this.wrapper.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
this.signature.v,
this.signature.r,
this.signature.s,
);

expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
expect(await this.token.allowance(owner, spender)).to.be.bignumber.equal(this.data.message.value);
});

it('revert on reused signature', async function () {
expect(await this.token.nonces(owner)).to.be.bignumber.equal('0');

await this.wrapper.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
this.signature.v,
this.signature.r,
this.signature.s,
);

expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');

await expectRevert(
this.wrapper.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
this.signature.v,
this.signature.r,
this.signature.s,
),
'SafeERC20: permit did not succeed',
);
expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
});

it('revert on invalid signature', async function () {
await expectRevert(
this.wrapper.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
27,
'0x71753dc5ecb5b4bfc0e3bc530d79ce5988760ed3f3a234c86a5546491f540775',
'0x0049cedee5aed990aabed5ad6a9f6e3c565b63379894b5fa8b512eb2b79e485d',
),
'SafeERC20: permit did not succeed',
);
});

it('underlying token does not revert on reused or invalid signature', async function () {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
await this.token.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
this.signature.v,
this.signature.r,
this.signature.s,
);
await this.token.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
this.signature.v,
this.signature.r,
this.signature.s,
);
await this.token.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
27,
'0x71753dc5ecb5b4bfc0e3bc530d79ce5988760ed3f3a234c86a5546491f540775',
'0x0049cedee5aed990aabed5ad6a9f6e3c565b63379894b5fa8b512eb2b79e485d',
);
});
});
});

function shouldRevertOnAllCalls (reason) {
Expand Down