From e123dd90ac186191a02d8af0d7d24476f4c87472 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Fri, 18 Jun 2021 14:57:27 +0200 Subject: [PATCH 1/2] Add magic salt to skip invalidation and allow only transferFrom selector --- contracts/LimitOrderProtocol.sol | 26 +++++++++++++------------- contracts/helpers/ERC1155Proxy.sol | 10 +++++----- contracts/helpers/ERC20Proxy.sol | 10 +++++----- contracts/helpers/ERC721Proxy.sol | 21 +++++++++++++-------- docgen/docs/helpers/ERC1155Proxy.md | 4 ++-- docgen/docs/helpers/ERC20Proxy.md | 4 ++-- docgen/docs/helpers/ERC721Proxy.md | 8 ++++---- test/LimitOrderProtocol.js | 20 ++++++++++++-------- 8 files changed, 56 insertions(+), 47 deletions(-) diff --git a/contracts/LimitOrderProtocol.sol b/contracts/LimitOrderProtocol.sol index ddfd4a86..ae56c6bc 100644 --- a/contracts/LimitOrderProtocol.sol +++ b/contracts/LimitOrderProtocol.sol @@ -4,13 +4,12 @@ pragma solidity ^0.8.0; import "@openzeppelin/contracts/interfaces/IERC1271.sol"; import "@openzeppelin/contracts/utils/math/SafeMath.sol"; +import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol"; import "./helpers/AmountCalculator.sol"; import "./helpers/ChainlinkCalculator.sol"; -import "./helpers/ERC1155Proxy.sol"; -import "./helpers/ERC20Proxy.sol"; -import "./helpers/ERC721Proxy.sol"; +import "./helpers/ImmutableOwner.sol"; import "./helpers/NonceManager.sol"; import "./helpers/PredicateHelper.sol"; import "./interfaces/InteractiveMaker.sol"; @@ -25,9 +24,6 @@ contract LimitOrderProtocol is EIP712("1inch Limit Order Protocol", "1"), AmountCalculator, ChainlinkCalculator, - ERC1155Proxy, - ERC20Proxy, - ERC721Proxy, NonceManager, PredicateHelper { @@ -82,8 +78,7 @@ contract LimitOrderProtocol is "OrderRFQ(uint256 info,address makerAsset,address takerAsset,bytes makerAssetData,bytes takerAssetData)" ); - // solhint-disable-next-line var-name-mixedcase - bytes4 immutable private _MAX_SELECTOR = bytes4(uint32(IERC20.transferFrom.selector) + 10); + uint256 constant public MAGIC_SALT_SKIPPING_INVALIDATION = 0xDEF1DEF1; uint256 constant private _FROM_INDEX = 0; uint256 constant private _TO_INDEX = 1; @@ -202,7 +197,7 @@ contract LimitOrderProtocol is uint256 expiration = uint128(order.info) >> 64; require(expiration == 0 || block.timestamp <= expiration, "LOP: order expired"); // solhint-disable-line not-rely-on-time - { // Stack too deep + if (uint64(order.info) != MAGIC_SALT_SKIPPING_INVALIDATION) { // Validate double spend address maker = order.makerAssetData.decodeAddress(_FROM_INDEX); uint256 invalidatorSlot = uint64(order.info) >> 8; @@ -287,7 +282,7 @@ contract LimitOrderProtocol is { // Stack too deep uint256 remainingMakerAmount; - { // Stack too deep + if (uint64(order.salt) != MAGIC_SALT_SKIPPING_INVALIDATION) { bool orderExists; (orderExists, remainingMakerAmount) = _remaining[orderHash].trySub(1); if (!orderExists) { @@ -300,6 +295,9 @@ contract LimitOrderProtocol is } } } + else { + remainingMakerAmount = order.makerAssetData.decodeUint256(_AMOUNT_INDEX); + } // Check if order is valid if (order.predicate.length > 0) { @@ -323,7 +321,9 @@ contract LimitOrderProtocol is // Update remaining amount in storage remainingMakerAmount = remainingMakerAmount.sub(makingAmount, "LOP: taking > remaining"); - _remaining[orderHash] = remainingMakerAmount + 1; + if (uint64(order.salt) != MAGIC_SALT_SKIPPING_INVALIDATION) { + _remaining[orderHash] = remainingMakerAmount + 1; + } emit OrderFilled(msg.sender, orderHash, remainingMakerAmount); } @@ -387,8 +387,8 @@ contract LimitOrderProtocol is require(takerAssetData.length >= 100, "LOP: bad takerAssetData.length"); bytes4 makerSelector = makerAssetData.decodeSelector(); bytes4 takerSelector = takerAssetData.decodeSelector(); - require(makerSelector >= IERC20.transferFrom.selector && makerSelector <= _MAX_SELECTOR, "LOP: bad makerAssetData.selector"); - require(takerSelector >= IERC20.transferFrom.selector && takerSelector <= _MAX_SELECTOR, "LOP: bad takerAssetData.selector"); + require(makerSelector == IERC20.transferFrom.selector, "LOP: bad makerAssetData.selector"); + require(takerSelector == IERC20.transferFrom.selector, "LOP: bad takerAssetData.selector"); address maker = address(makerAssetData.decodeAddress(_FROM_INDEX)); if ((signature.length != 65 && signature.length != 64) || SilentECDSA.recover(orderHash, signature) != maker) { diff --git a/contracts/helpers/ERC1155Proxy.sol b/contracts/helpers/ERC1155Proxy.sol index 68a88afd..e432695c 100644 --- a/contracts/helpers/ERC1155Proxy.sol +++ b/contracts/helpers/ERC1155Proxy.sol @@ -10,13 +10,13 @@ import "./ImmutableOwner.sol"; /* solhint-disable func-name-mixedcase */ -abstract contract ERC1155Proxy is ImmutableOwner { - constructor() { - require(ERC1155Proxy.func_733NCGU.selector == bytes4(uint32(IERC20.transferFrom.selector) + 4), "ERC1155Proxy: bad selector"); +contract ERC1155Proxy is ImmutableOwner { + constructor(address _immutableOwner) ImmutableOwner(_immutableOwner) { + require(ERC1155Proxy.func_301JL5R.selector == bytes4(uint32(IERC20.transferFrom.selector) + 4), "ERC1155Proxy: bad selector"); } - // keccak256("func_733NCGU(address,address,uint256,address,uint256,bytes)") == 0x23b872e1 - function func_733NCGU(address from, address to, uint256 amount, IERC1155 token, uint256 tokenId, bytes calldata data) external onlyImmutableOwner { + // keccak256("func_301JL5R(address,address,uint256,address,uint256,bytes)") == 0x23b872e1 + function func_301JL5R(address from, address to, uint256 amount, IERC1155 token, uint256 tokenId, bytes calldata data) external onlyImmutableOwner { token.safeTransferFrom(from, to, tokenId, amount, data); } } diff --git a/contracts/helpers/ERC20Proxy.sol b/contracts/helpers/ERC20Proxy.sol index 045eda73..51d6578f 100644 --- a/contracts/helpers/ERC20Proxy.sol +++ b/contracts/helpers/ERC20Proxy.sol @@ -9,15 +9,15 @@ import "./ImmutableOwner.sol"; /* solhint-disable func-name-mixedcase */ -abstract contract ERC20Proxy is ImmutableOwner { +contract ERC20Proxy is ImmutableOwner { using SafeERC20 for IERC20; - constructor() { - require(ERC20Proxy.func_50BkM4K.selector == bytes4(uint32(IERC20.transferFrom.selector) + 1), "ERC20Proxy: bad selector"); + constructor(address _immutableOwner) ImmutableOwner(_immutableOwner) { + require(ERC20Proxy.func_602HzuS.selector == IERC20.transferFrom.selector, "ERC20Proxy: bad selector"); } - // keccak256("func_50BkM4K(address,address,uint256,address)") = 0x23b872de - function func_50BkM4K(address from, address to, uint256 amount, IERC20 token) external onlyImmutableOwner { + // keccak256("func_602HzuS(address,address,uint256,address)") = 0x23b872dd + function func_602HzuS(address from, address to, uint256 amount, IERC20 token) external onlyImmutableOwner { token.safeTransferFrom(from, to, amount); } } diff --git a/contracts/helpers/ERC721Proxy.sol b/contracts/helpers/ERC721Proxy.sol index f0ba055c..1e798e5a 100644 --- a/contracts/helpers/ERC721Proxy.sol +++ b/contracts/helpers/ERC721Proxy.sol @@ -10,19 +10,24 @@ import "./ImmutableOwner.sol"; /* solhint-disable func-name-mixedcase */ -abstract contract ERC721Proxy is ImmutableOwner { - constructor() { - require(ERC721Proxy.func_40aVqeY.selector == bytes4(uint32(IERC20.transferFrom.selector) + 2), "ERC20Proxy: bad selector"); - require(ERC721Proxy.func_20xtkDI.selector == bytes4(uint32(IERC20.transferFrom.selector) + 3), "ERC20Proxy: bad selector"); +contract ERC721Proxy is ImmutableOwner { + constructor(address _immutableOwner) ImmutableOwner(_immutableOwner) { + require(ERC721Proxy.func_602HzuS.selector == IERC20.transferFrom.selector, "ERC20Proxy: bad selector"); } - // keccak256("func_40aVqeY(address,address,uint256,address)") == 0x23b872df - function func_40aVqeY(address from, address to, uint256 tokenId, IERC721 token) external onlyImmutableOwner { + // keccak256("func_602HzuS(address,address,uint256,address)") == 0x23b872dd + function func_602HzuS(address from, address to, uint256 tokenId, IERC721 token) external onlyImmutableOwner { token.transferFrom(from, to, tokenId); } +} + +contract ERC721ProxySafe is ImmutableOwner { + constructor(address _immutableOwner) ImmutableOwner(_immutableOwner) { + require(ERC721ProxySafe.func_602HzuS.selector == IERC20.transferFrom.selector, "ERC20Proxy: bad selector"); + } - // keccak256("func_20xtkDI(address,address,uint256,address)" == 0x23b872e0 - function func_20xtkDI(address from, address to, uint256 tokenId, IERC721 token) external onlyImmutableOwner { + // keccak256("func_602HzuS(address,address,uint256,address)" == 0x23b872e0 + function func_602HzuS(address from, address to, uint256 tokenId, IERC721 token) external onlyImmutableOwner { token.safeTransferFrom(from, to, tokenId); } } diff --git a/docgen/docs/helpers/ERC1155Proxy.md b/docgen/docs/helpers/ERC1155Proxy.md index a66a0089..7c6aadee 100644 --- a/docgen/docs/helpers/ERC1155Proxy.md +++ b/docgen/docs/helpers/ERC1155Proxy.md @@ -14,9 +14,9 @@ function constructor( -### func_733NCGU +### func_301JL5R ```solidity -function func_733NCGU( +function func_301JL5R( address from, address to, uint256 amount, diff --git a/docgen/docs/helpers/ERC20Proxy.md b/docgen/docs/helpers/ERC20Proxy.md index ffef0de6..0c62a6f5 100644 --- a/docgen/docs/helpers/ERC20Proxy.md +++ b/docgen/docs/helpers/ERC20Proxy.md @@ -14,9 +14,9 @@ function constructor( -### func_50BkM4K +### func_602HzuS ```solidity -function func_50BkM4K( +function func_602HzuS( address from, address to, uint256 amount, diff --git a/docgen/docs/helpers/ERC721Proxy.md b/docgen/docs/helpers/ERC721Proxy.md index d1b5cbfc..ace5048c 100644 --- a/docgen/docs/helpers/ERC721Proxy.md +++ b/docgen/docs/helpers/ERC721Proxy.md @@ -14,9 +14,9 @@ function constructor( -### func_40aVqeY +### func_602HzuS ```solidity -function func_40aVqeY( +function func_602HzuS( address from, address to, uint256 tokenId, @@ -34,9 +34,9 @@ function func_40aVqeY( |`token` | contract IERC721 | -### func_20xtkDI +### func_602HzuS ```solidity -function func_20xtkDI( +function func_602HzuS( address from, address to, uint256 tokenId, diff --git a/test/LimitOrderProtocol.js b/test/LimitOrderProtocol.js index 42ab6e99..d8c38ecf 100644 --- a/test/LimitOrderProtocol.js +++ b/test/LimitOrderProtocol.js @@ -7,6 +7,7 @@ const Wallet = require('ethereumjs-wallet').default; const TokenMock = artifacts.require('TokenMock'); const LimitOrderProtocol = artifacts.require('LimitOrderProtocol'); +const ERC20Proxy = artifacts.require('ERC20Proxy'); const { profileEVM, gasspectEVM } = require('./helpers/profileEVM'); const { buildOrderData, buildOrderRFQData } = require('./helpers/orderUtils'); @@ -52,6 +53,7 @@ contract('LimitOrderProtocol', async function ([_, wallet]) { this.weth = await TokenMock.new('WETH', 'WETH'); this.swap = await LimitOrderProtocol.new(); + this.erc20proxy = await ERC20Proxy.new(this.swap.address); // We get the chain id from the contract because Ganache (used for coverage) does not return the same chain id // from within the EVM as from the JSON RPC interface. @@ -63,10 +65,12 @@ contract('LimitOrderProtocol', async function ([_, wallet]) { await this.dai.mint(_, '1000000'); await this.weth.mint(_, '1000000'); - await this.dai.approve(this.swap.address, '1000000'); - await this.weth.approve(this.swap.address, '1000000'); - await this.dai.approve(this.swap.address, '1000000', { from: wallet }); - await this.weth.approve(this.swap.address, '1000000', { from: wallet }); + for (let to of [this.swap.address, this.erc20proxy.address]) { + await this.dai.approve(to, '1000000'); + await this.weth.approve(to, '1000000'); + await this.dai.approve(to, '1000000', { from: wallet }); + await this.weth.approve(to, '1000000', { from: wallet }); + } }); describe('wip', async function () { @@ -271,10 +275,10 @@ contract('LimitOrderProtocol', async function ([_, wallet]) { it('ERC20Proxy should work', async function () { const order = buildOrder(this.swap, this.dai, this.weth, 10, 10); - this.makerAsset = this.swap.address; - this.takerAsset = this.swap.address; - this.makerAssetData = this.swap.contract.methods.func_50BkM4K(wallet, zeroAddress, 10, this.dai.address).encodeABI(); - this.takerAssetData = this.swap.contract.methods.func_50BkM4K(zeroAddress, wallet, 10, this.weth.address).encodeABI(); + this.makerAsset = this.erc20proxy.address; + this.takerAsset = this.erc20proxy.address; + this.makerAssetData = this.erc20proxy.contract.methods.func_602HzuS(wallet, zeroAddress, 10, this.dai.address).encodeABI(); + this.takerAssetData = this.erc20proxy.contract.methods.func_602HzuS(zeroAddress, wallet, 10, this.weth.address).encodeABI(); const data = buildOrderData(this.chainId, this.swap.address, order); const signature = ethSigUtil.signTypedMessage(account.getPrivateKey(), { data }); From 9de208c7313a0d53d6ad247a51f5b47180e04cf1 Mon Sep 17 00:00:00 2001 From: Mikhail Melnik Date: Mon, 27 Sep 2021 14:06:23 +0800 Subject: [PATCH 2/2] wip --- docgen/docs/helpers/ERC1155Proxy.md | 40 --------------------- docgen/docs/helpers/ERC20Proxy.md | 36 ------------------- docgen/docs/helpers/ERC721Proxy.md | 56 ----------------------------- docgen/docs/helpers/erc1155proxy.md | 40 --------------------- docgen/docs/helpers/erc20proxy.md | 36 ------------------- docgen/docs/helpers/erc721proxy.md | 56 ----------------------------- 6 files changed, 264 deletions(-) delete mode 100644 docgen/docs/helpers/ERC1155Proxy.md delete mode 100644 docgen/docs/helpers/ERC20Proxy.md delete mode 100644 docgen/docs/helpers/ERC721Proxy.md delete mode 100644 docgen/docs/helpers/erc1155proxy.md delete mode 100644 docgen/docs/helpers/erc20proxy.md delete mode 100644 docgen/docs/helpers/erc721proxy.md diff --git a/docgen/docs/helpers/ERC1155Proxy.md b/docgen/docs/helpers/ERC1155Proxy.md deleted file mode 100644 index 7c6aadee..00000000 --- a/docgen/docs/helpers/ERC1155Proxy.md +++ /dev/null @@ -1,40 +0,0 @@ -# ERC1155Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_301JL5R -```solidity -function func_301JL5R( - address from, - address to, - uint256 amount, - contract IERC1155 token, - uint256 tokenId, - bytes data -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`amount` | uint256 | -|`token` | contract IERC1155 | -|`tokenId` | uint256 | -|`data` | bytes | - - diff --git a/docgen/docs/helpers/ERC20Proxy.md b/docgen/docs/helpers/ERC20Proxy.md deleted file mode 100644 index 0c62a6f5..00000000 --- a/docgen/docs/helpers/ERC20Proxy.md +++ /dev/null @@ -1,36 +0,0 @@ -# ERC20Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_602HzuS -```solidity -function func_602HzuS( - address from, - address to, - uint256 amount, - contract IERC20 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`amount` | uint256 | -|`token` | contract IERC20 | - - diff --git a/docgen/docs/helpers/ERC721Proxy.md b/docgen/docs/helpers/ERC721Proxy.md deleted file mode 100644 index ace5048c..00000000 --- a/docgen/docs/helpers/ERC721Proxy.md +++ /dev/null @@ -1,56 +0,0 @@ -# ERC721Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_602HzuS -```solidity -function func_602HzuS( - address from, - address to, - uint256 tokenId, - contract IERC721 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`tokenId` | uint256 | -|`token` | contract IERC721 | - - -### func_602HzuS -```solidity -function func_602HzuS( - address from, - address to, - uint256 tokenId, - contract IERC721 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`tokenId` | uint256 | -|`token` | contract IERC721 | - - diff --git a/docgen/docs/helpers/erc1155proxy.md b/docgen/docs/helpers/erc1155proxy.md deleted file mode 100644 index a66a0089..00000000 --- a/docgen/docs/helpers/erc1155proxy.md +++ /dev/null @@ -1,40 +0,0 @@ -# ERC1155Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_733NCGU -```solidity -function func_733NCGU( - address from, - address to, - uint256 amount, - contract IERC1155 token, - uint256 tokenId, - bytes data -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`amount` | uint256 | -|`token` | contract IERC1155 | -|`tokenId` | uint256 | -|`data` | bytes | - - diff --git a/docgen/docs/helpers/erc20proxy.md b/docgen/docs/helpers/erc20proxy.md deleted file mode 100644 index ffef0de6..00000000 --- a/docgen/docs/helpers/erc20proxy.md +++ /dev/null @@ -1,36 +0,0 @@ -# ERC20Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_50BkM4K -```solidity -function func_50BkM4K( - address from, - address to, - uint256 amount, - contract IERC20 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`amount` | uint256 | -|`token` | contract IERC20 | - - diff --git a/docgen/docs/helpers/erc721proxy.md b/docgen/docs/helpers/erc721proxy.md deleted file mode 100644 index d1b5cbfc..00000000 --- a/docgen/docs/helpers/erc721proxy.md +++ /dev/null @@ -1,56 +0,0 @@ -# ERC721Proxy - - - - - -## Functions -### constructor -```solidity -function constructor( -) internal -``` - - - - -### func_40aVqeY -```solidity -function func_40aVqeY( - address from, - address to, - uint256 tokenId, - contract IERC721 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`tokenId` | uint256 | -|`token` | contract IERC721 | - - -### func_20xtkDI -```solidity -function func_20xtkDI( - address from, - address to, - uint256 tokenId, - contract IERC721 token -) external -``` - - -#### Parameters: -| Name | Type | Description | -| :--- | :--- | :------------------------------------------------------------------- | -|`from` | address | -|`to` | address | -|`tokenId` | uint256 | -|`token` | contract IERC721 | - -