From 437a3b048d2ca0b489fe581acd2b4578c7a557f8 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 16 Dec 2020 01:37:39 -0500 Subject: [PATCH] EP: Address audit feedback (#82) * `@0x/contracts-zero-ex`: Address audit feedback (1/2) * `@0x/contracts-zero-ex`: Cap the ETH transfer amount to a liquidity provider to `msg.value` * `@0x/contracts-zero-ex`: Bump feature contract versions * `@0x/contracts-zero-ex`: Always transfer msg.value to the liqudity provider in LiquiidityProviderFeature * Remove PLP backwards-compatibility (#85) * Remove backwards-compatibility from MixinZeroExBridge and LiquidityProviderSandbox * `@0x/contracts-zero-ex`: Update CHANGELOG Co-authored-by: Lawrence Forman Co-authored-by: Lawrence Forman Co-authored-by: mzhu25 --- contracts/zero-ex/CHANGELOG.json | 17 ++++++++ .../contracts/src/external/FeeCollector.sol | 1 - .../src/external/LiquidityProviderSandbox.sol | 12 +----- .../src/features/LiquidityProviderFeature.sol | 12 ++++-- .../contracts/src/features/UniswapFeature.sol | 19 ++++++-- .../bridges/mixins/MixinZeroExBridge.sol | 43 +++++++------------ .../test/features/liquidity_provider_test.ts | 37 ---------------- 7 files changed, 57 insertions(+), 84 deletions(-) diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index ebf8b74e1e..113521337f 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -1,4 +1,21 @@ [ + { + "version": "0.13.0", + "changes": [ + { + "note": "Address audit feedback in UniswapFeature", + "pr": 82 + }, + { + "note": "Always transfer `msg.value` to the liquidity provider contract in LiquidityProviderFeature to", + "pr": 82 + }, + { + "note": "Remove backwards compatibility with old PLP/bridge interface in `LiquidityProviderFeature` and `MixinZeroExBridge`", + "pr": 85 + } + ] + }, { "version": "0.12.0", "changes": [ diff --git a/contracts/zero-ex/contracts/src/external/FeeCollector.sol b/contracts/zero-ex/contracts/src/external/FeeCollector.sol index 38e128518a..8e815afc48 100644 --- a/contracts/zero-ex/contracts/src/external/FeeCollector.sol +++ b/contracts/zero-ex/contracts/src/external/FeeCollector.sol @@ -57,7 +57,6 @@ contract FeeCollector is AuthorizableV06 { external onlyAuthorized { - // Leave 1 wei behind to avoid expensive zero-->non-zero state change. if (address(this).balance > 0) { weth.deposit{value: address(this).balance}(); } diff --git a/contracts/zero-ex/contracts/src/external/LiquidityProviderSandbox.sol b/contracts/zero-ex/contracts/src/external/LiquidityProviderSandbox.sol index 30fea8ec71..b9d7e2c2df 100644 --- a/contracts/zero-ex/contracts/src/external/LiquidityProviderSandbox.sol +++ b/contracts/zero-ex/contracts/src/external/LiquidityProviderSandbox.sol @@ -68,21 +68,13 @@ contract LiquidityProviderSandbox is onlyOwner override { - try ILiquidityProvider(provider).sellTokenForToken( + ILiquidityProvider(provider).sellTokenForToken( inputToken, outputToken, recipient, minBuyAmount, auxiliaryData - ) {} catch { - IERC20Bridge(provider).bridgeTransferFrom( - outputToken, - provider, - recipient, - minBuyAmount, - auxiliaryData - ); - } + ); } /// @dev Calls `sellEthForToken` on the given `provider` contract to diff --git a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol index 1e9e0271a6..5090286ec9 100644 --- a/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol +++ b/contracts/zero-ex/contracts/src/features/LiquidityProviderFeature.sol @@ -44,7 +44,7 @@ contract LiquidityProviderFeature is /// @dev Name of this feature. string public constant override FEATURE_NAME = "LiquidityProviderFeature"; /// @dev Version of this feature. - uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 0, 1); + uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 0, 2); /// @dev ETH pseudo-token address. address constant internal ETH_TOKEN_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; @@ -111,9 +111,13 @@ contract LiquidityProviderFeature is recipient = msg.sender; } - if (inputToken == ETH_TOKEN_ADDRESS) { - provider.transfer(sellAmount); - } else { + // Forward all attached ETH to the provider. + if (msg.value > 0) { + provider.transfer(msg.value); + } + + if (inputToken != ETH_TOKEN_ADDRESS) { + // Transfer input ERC20 tokens to the provider. _transferERC20Tokens( IERC20TokenV06(inputToken), msg.sender, diff --git a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol index 9ea99f0eb6..c9b3ea12b5 100644 --- a/contracts/zero-ex/contracts/src/features/UniswapFeature.sol +++ b/contracts/zero-ex/contracts/src/features/UniswapFeature.sol @@ -37,7 +37,7 @@ contract UniswapFeature is /// @dev Name of this feature. string public constant override FEATURE_NAME = "UniswapFeature"; /// @dev Version of this feature. - uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 1, 0); + uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 1, 1); /// @dev A bloom filter for tokens that consume all gas when `transferFrom()` fails. bytes32 public immutable GREEDY_TOKENS_BLOOM_FILTER; /// @dev WETH contract. @@ -167,8 +167,13 @@ contract UniswapFeature is } if iszero(i) { + // This is the first token in the path. switch eq(sellToken, ETH_TOKEN_ADDRESS_32) case 0 { // Not selling ETH. Selling an ERC20 instead. + // Make sure ETH was not attached to the call. + if gt(callvalue(), 0) { + revert(0, 0) + } // For the first pair we need to transfer sellTokens into the // pair contract. moveTakerTokensTo(sellToken, pair, sellAmount) @@ -203,6 +208,10 @@ contract UniswapFeature is if iszero(staticcall(gas(), pair, 0xB00, 0x4, 0xC00, 0x40)) { bubbleRevert() } + // Revert if the pair contract does not return two words. + if iszero(eq(returndatasize(), 0x40)) { + revert(0,0) + } // Sell amount for this hop is the previous buy amount. let pairSellAmount := buyAmount @@ -374,11 +383,15 @@ contract UniswapFeature is mstore(0xB00, ALLOWANCE_CALL_SELECTOR_32) mstore(0xB04, caller()) mstore(0xB24, address()) - let success := call(gas(), token, 0, 0xB00, 0x44, 0xC00, 0x20) + let success := staticcall(gas(), token, 0xB00, 0x44, 0xC00, 0x20) if iszero(success) { // Call to allowance() failed. bubbleRevert() } + // Make sure the allowance call returned a single word. + if iszero(eq(returndatasize(), 0x20)) { + revert(0, 0) + } // Call succeeded. // Result is stored in 0xC00-0xC20. if lt(mload(0xC00), amount) { @@ -397,8 +410,6 @@ contract UniswapFeature is mstore(0xB44, amount) let success := call( - // Cap the gas limit to prvent all gas being consumed - // if the token reverts. gas(), token, 0, diff --git a/contracts/zero-ex/contracts/src/transformers/bridges/mixins/MixinZeroExBridge.sol b/contracts/zero-ex/contracts/src/transformers/bridges/mixins/MixinZeroExBridge.sol index b2e1cd8e87..39a52162f1 100644 --- a/contracts/zero-ex/contracts/src/transformers/bridges/mixins/MixinZeroExBridge.sol +++ b/contracts/zero-ex/contracts/src/transformers/bridges/mixins/MixinZeroExBridge.sol @@ -22,7 +22,6 @@ import "@0x/contracts-erc20/contracts/src/v06/LibERC20TokenV06.sol"; import "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; import "@0x/contracts-utils/contracts/src/v06/LibSafeMathV06.sol"; import "../../../vendor/ILiquidityProvider.sol"; -import "../../../vendor/v3/IERC20Bridge.sol"; contract MixinZeroExBridge { @@ -61,32 +60,20 @@ contract MixinZeroExBridge { bridgeAddress, sellAmount ); - try ILiquidityProvider(bridgeAddress).sellTokenForToken( - address(sellToken), - address(buyToken), - address(this), // recipient - 1, // minBuyAmount - bridgeData - ) returns (uint256 _boughtAmount) { - boughtAmount = _boughtAmount; - emit ERC20BridgeTransfer( - sellToken, - buyToken, - sellAmount, - boughtAmount, - bridgeAddress, - address(this) - ); - } catch { - uint256 balanceBefore = buyToken.balanceOf(address(this)); - IERC20Bridge(bridgeAddress).bridgeTransferFrom( - address(buyToken), - bridgeAddress, - address(this), // recipient - 1, // minBuyAmount - bridgeData - ); - boughtAmount = buyToken.balanceOf(address(this)).safeSub(balanceBefore); - } + boughtAmount = ILiquidityProvider(bridgeAddress).sellTokenForToken( + address(sellToken), + address(buyToken), + address(this), // recipient + 1, // minBuyAmount + bridgeData + ); + emit ERC20BridgeTransfer( + sellToken, + buyToken, + sellAmount, + boughtAmount, + bridgeAddress, + address(this) + ); } } diff --git a/contracts/zero-ex/test/features/liquidity_provider_test.ts b/contracts/zero-ex/test/features/liquidity_provider_test.ts index 88a4368c67..de5647cf1c 100644 --- a/contracts/zero-ex/test/features/liquidity_provider_test.ts +++ b/contracts/zero-ex/test/features/liquidity_provider_test.ts @@ -9,8 +9,6 @@ import { abis } from '../utils/abis'; import { fullMigrateAsync } from '../utils/migration'; import { LiquidityProviderSandboxContract, - TestBridgeContract, - TestBridgeEvents, TestLiquidityProviderContract, TestLiquidityProviderEvents, TestWethContract, @@ -148,41 +146,6 @@ blockchainTests('LiquidityProvider feature', env => { TestLiquidityProviderEvents.SellTokenForToken, ); }); - it('Successfully executes an ERC20-ERC20 swap (backwards-compatibility)', async () => { - const bridge = await TestBridgeContract.deployFrom0xArtifactAsync( - artifacts.TestBridge, - env.provider, - env.txDefaults, - artifacts, - weth.address, - token.address, - ); - const tx = await feature - .sellToLiquidityProvider( - token.address, - weth.address, - bridge.address, - constants.NULL_ADDRESS, - constants.ONE_ETHER, - constants.ZERO_AMOUNT, - constants.NULL_BYTES, - ) - .awaitTransactionSuccessAsync({ from: taker }); - verifyEventsFromLogs( - tx.logs, - [ - { - inputToken: token.address, - outputToken: weth.address, - inputTokenAmount: constants.ONE_ETHER, - outputTokenAmount: constants.ZERO_AMOUNT, - from: bridge.address, - to: taker, - }, - ], - TestBridgeEvents.ERC20BridgeTransfer, - ); - }); it('Reverts if cannot fulfill the minimum buy amount', async () => { const minBuyAmount = new BigNumber(1); const tx = feature