diff --git a/CHANGELOG.md b/CHANGELOG.md index f30f4174300..f8f9d6f25a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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` as mitigation against phantom permit functions. ([#3280](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3280)) * `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)) diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol index f3bcc397261..af3420e3d17 100644 --- a/contracts/mocks/SafeERC20Helper.sol +++ b/contracts/mocks/SafeERC20Helper.sol @@ -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 { @@ -105,6 +106,43 @@ contract ERC20NoReturnMock is Context { } } +contract ERC20PermitNoRevertMock is + ERC20("ERC20PermitNoRevertMock", "ERC20PermitNoRevertMock"), + ERC20Permit("ERC20PermitNoRevertMock") +{ + function getChainId() external view returns (uint256) { + return block.chainid; + } + + 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 + } + } +} + contract SafeERC20Wrapper is Context { using SafeERC20 for IERC20; @@ -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 { + SafeERC20.safePermit(IERC20Permit(address(_token)), owner, spender, value, deadline, v, r, s); + } + function setAllowance(uint256 allowance_) public { ERC20ReturnTrueMock(address(_token)).setAllowance(allowance_); } diff --git a/contracts/token/ERC20/utils/SafeERC20.sol b/contracts/token/ERC20/utils/SafeERC20.sol index 5752d93138e..31053ee7d42 100644 --- a/contracts/token/ERC20/utils/SafeERC20.sol +++ b/contracts/token/ERC20/utils/SafeERC20.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.0; import "../IERC20.sol"; +import "../extensions/draft-IERC20Permit.sol"; import "../../../utils/Address.sol"; /** @@ -79,6 +80,22 @@ 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); + token.permit(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). diff --git a/test/helpers/eip712.js b/test/helpers/eip712.js index 4f8fe7dfb27..8a64b8a35bd 100644 --- a/test/helpers/eip712.js +++ b/test/helpers/eip712.js @@ -7,6 +7,14 @@ const EIP712Domain = [ { name: 'verifyingContract', type: 'address' }, ]; +const Permit = [ + { name: 'owner', type: 'address' }, + { name: 'spender', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint256' }, +]; + async function domainSeparator (name, version, chainId, verifyingContract) { return '0x' + ethSigUtil.TypedDataUtils.hashStruct( 'EIP712Domain', @@ -17,5 +25,6 @@ async function domainSeparator (name, version, chainId, verifyingContract) { module.exports = { EIP712Domain, + Permit, domainSeparator, }; diff --git a/test/token/ERC20/extensions/draft-ERC20Permit.test.js b/test/token/ERC20/extensions/draft-ERC20Permit.test.js index 9aa64456110..fb60a6cbe52 100644 --- a/test/token/ERC20/extensions/draft-ERC20Permit.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Permit.test.js @@ -10,15 +10,7 @@ const Wallet = require('ethereumjs-wallet').default; const ERC20PermitMock = artifacts.require('ERC20PermitMock'); -const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); - -const Permit = [ - { name: 'owner', type: 'address' }, - { name: 'spender', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'deadline', type: 'uint256' }, -]; +const { EIP712Domain, Permit, domainSeparator } = require('../../../helpers/eip712'); contract('ERC20Permit', function (accounts) { const [ initialHolder, spender, recipient, other ] = accounts; diff --git a/test/token/ERC20/utils/SafeERC20.test.js b/test/token/ERC20/utils/SafeERC20.test.js index 0bed7052101..4c54c583059 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/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, Permit } = 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; @@ -39,6 +46,119 @@ contract('SafeERC20', function (accounts) { shouldOnlyRevertOnErrors(); }); + + 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(); + 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'); + // use valid signature and consume nounce + 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'); + // 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 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 () { + // 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 expectRevert( + this.wrapper.permit( + this.data.message.owner, + this.data.message.spender, + this.data.message.value, + this.data.message.deadline, + invalidSignature.v, + invalidSignature.r, + invalidSignature.s, + ), + 'SafeERC20: permit did not succeed', + ); + }); + }); }); function shouldRevertOnAllCalls (reason) {