NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove SafeERC20.safePermit (#4582)

Browse Source 
Co-authored-by: Francisco <fg@frang.io>
This commit is contained in:
Hadrien Croubois
2023-09-11 17:07:25 +02:00
committed by GitHub
parent 9ef69c03d1
commit 095c8e120c
7 changed files with 43 additions and 186 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/green-pumpkins-end.md Normal file
View File

@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---
`SafeERC20`: Removed `safePermit` in favor of documentation-only `permit` recommendations.

35
contracts/mocks/token/ERC20PermitNoRevertMock.sol
View File

@ -1,35 +0,0 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
import {ERC20Permit} from "../../token/ERC20/extensions/ERC20Permit.sol";
abstract contract ERC20PermitNoRevertMock is ERC20Permit {
function permitThatMayRevert(
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.permitThatMayRevert(owner, spender, value, deadline, v, r, s) {
// do nothing
} catch {
// do nothing
}
}
}

8
contracts/token/ERC20/README.adoc
View File

@ -15,10 +15,10 @@ There are a few core contracts that implement the behavior specified in the EIP:
Additionally there are multiple custom extensions, including:
* {ERC20Permit}: gasless approval of tokens (standardized as ERC2612).
* {ERC20Burnable}: destruction of own tokens.
* {ERC20Capped}: enforcement of a cap to the total supply when minting tokens.
* {ERC20Pausable}: ability to pause token transfers.
* {ERC20Permit}: gasless approval of tokens (standardized as ERC2612).
* {ERC20FlashMint}: token level support for flash loans through the minting and burning of ephemeral tokens (standardized as ERC3156).
* {ERC20Votes}: support for voting and vote delegation.
* {ERC20Wrapper}: wrapper to create an ERC20 backed by another ERC20, with deposit and withdraw methods. Useful in conjunction with {ERC20Votes}.
@ -44,14 +44,16 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel
== Extensions
{{IERC20Permit}}
{{ERC20Permit}}
{{ERC20Burnable}}
{{ERC20Capped}}
{{ERC20Pausable}}
{{ERC20Permit}}
{{ERC20Votes}}
{{ERC20Wrapper}}

6
contracts/token/ERC20/extensions/ERC20Permit.sol
View File

@ -39,7 +39,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces {
constructor(string memory name) EIP712(name, "1") {}
/**
* @dev See {IERC20Permit-permit}.
* @inheritdoc IERC20Permit
*/
function permit(
address owner,
@ -67,14 +67,14 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces {
}
/**
* @dev See {IERC20Permit-nonces}.
* @inheritdoc IERC20Permit
*/
function nonces(address owner) public view virtual override(IERC20Permit, Nonces) returns (uint256) {
return super.nonces(owner);
}
/**
* @dev See {IERC20Permit-DOMAIN_SEPARATOR}.
* @inheritdoc IERC20Permit
*/
// solhint-disable-next-line func-name-mixedcase
function DOMAIN_SEPARATOR() external view virtual returns (bytes32) {

30
contracts/token/ERC20/extensions/IERC20Permit.sol
View File

@ -10,6 +10,34 @@ pragma solidity ^0.8.20;
* Adds the {permit} method, which can be used to change an account's ERC20 allowance (see {IERC20-allowance}) by
* 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.
*
* ==== Security Considerations
*
* There are two important considerations concerning the use of `permit`. The first is that a valid permit signature
* expresses an allowance, and it should not be assumed to convey additional meaning. In particular, it should not be
* considered as an intention to spend the allowance in any specific way. The second is that because permits have
* built-in replay protection and can be submitted by anyone, they can be frontrun. A protocol that uses permits should
* take this into consideration and allow a `permit` call to fail. Combining these two aspects, a pattern that may be
* generally recommended is:
*
* ```solidity
* function doThingWithPermit(..., uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) public {
* try token.permit(msg.sender, address(this), value, deadline, v, r, s) {} catch {}
* doThing(..., value);
* }
*
* function doThing(..., uint256 value) public {
* token.safeTransferFrom(msg.sender, address(this), value);
* ...
* }
* ```
*
* Observe that: 1) `msg.sender` is used as the owner, leaving no ambiguity as to the signer intent, and 2) the use of
* `try/catch` allows the permit to fail and makes the code tolerant to frontrunning. (See also
* {SafeERC20-safeTransferFrom}).
*
* Additionally, note that smart contract wallets (such as Argent or Safe) are not able to produce permit signatures, so
* contracts should have entry points that don't rely on permit.
*/
interface IERC20Permit {
/**
@ -32,6 +60,8 @@ interface IERC20Permit {
* For more information on the signature format, see the
* https://eips.ethereum.org/EIPS/eip-2612#specification[relevant EIP
* section].
*
* CAUTION: See Security Considerations above.
*/
function permit(
address owner,

22
contracts/token/ERC20/utils/SafeERC20.sol
View File

@ -82,28 +82,6 @@ library SafeERC20 {
}
}
/**
* @dev Use a ERC-2612 signature to set the `owner` approval toward `spender` on `token`.
* Revert on invalid signature.
*/
function safePermit(
IERC20Permit token,
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) internal {
uint256 nonceBefore = token.nonces(owner);
token.permit(owner, spender, value, deadline, v, r, s);
uint256 nonceAfter = token.nonces(owner);
if (nonceAfter != nonceBefore + 1) {
revert SafeERC20FailedOperation(address(token));
}
}
/**
* @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).

123
test/token/ERC20/utils/SafeERC20.test.js
View File

@ -4,16 +4,10 @@ const SafeERC20 = artifacts.require('$SafeERC20');
const ERC20ReturnFalseMock = artifacts.require('$ERC20ReturnFalseMock');
const ERC20ReturnTrueMock = artifacts.require('$ERC20'); // default implementation returns true
const ERC20NoReturnMock = artifacts.require('$ERC20NoReturnMock');
const ERC20PermitNoRevertMock = artifacts.require('$ERC20PermitNoRevertMock');
const ERC20ForceApproveMock = artifacts.require('$ERC20ForceApproveMock');
const { getDomain, domainType, Permit } = require('../../../helpers/eip712');
const { expectRevertCustomError } = require('../../../helpers/customError');
const { fromRpcSig } = require('ethereumjs-util');
const ethSigUtil = require('eth-sig-util');
const Wallet = require('ethereumjs-wallet').default;
const name = 'ERC20Mock';
const symbol = 'ERC20Mock';
@ -122,123 +116,6 @@ contract('SafeERC20', function (accounts) {
shouldOnlyRevertOnErrors(accounts);
});
describe("with token that doesn't revert on invalid permit", function () {
const wallet = Wallet.generate();
const owner = wallet.getAddressString();
const spender = hasNoCode;
beforeEach(async function () {
this.token = await ERC20PermitNoRevertMock.new(name, symbol, name);
this.data = await getDomain(this.token).then(domain => ({
primaryType: 'Permit',
types: { EIP712Domain: domainType(domain), Permit },
domain,
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.mock.$safePermit(
this.token.address,
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');
// use valid signature and consume nounce
await this.mock.$safePermit(
this.token.address,
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');
// invalid call does not revert for this token implementation
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,
);
expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
// invalid call revert when called through the SafeERC20 library
await expectRevertCustomError(
this.mock.$safePermit(
this.token.address,
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,
),
'SafeERC20FailedOperation',
[this.token.address],
);
expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
});
it('revert on invalid signature', async function () {
// signature that is not valid for owner
const invalidSignature = {
v: 27,
r: '0x71753dc5ecb5b4bfc0e3bc530d79ce5988760ed3f3a234c86a5546491f540775',
s: '0x0049cedee5aed990aabed5ad6a9f6e3c565b63379894b5fa8b512eb2b79e485d',
};
// invalid call does not revert for this token implementation
await this.token.permit(
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
invalidSignature.v,
invalidSignature.r,
invalidSignature.s,
);
// invalid call revert when called through the SafeERC20 library
await expectRevertCustomError(
this.mock.$safePermit(
this.token.address,
this.data.message.owner,
this.data.message.spender,
this.data.message.value,
this.data.message.deadline,
invalidSignature.v,
invalidSignature.r,
invalidSignature.s,
),
'SafeERC20FailedOperation',
[this.token.address],
);
});
});
describe('with usdt approval beaviour', function () {
const spender = hasNoCode;