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 <me@merklejerk.com> Co-authored-by: Lawrence Forman <me@merklejerk.com> Co-authored-by: mzhu25 <mchl.zhu.96@gmail.com>
2020-12-16 01:37:39 -05:00 · 2020-12-16 01:37:39 -05:00 · 437a3b048d
commit 437a3b048d
parent f55a9454b5
7 changed files with 57 additions and 84 deletions
--- 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": [
--- 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}();
        }
--- 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
--- 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,
--- 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,
--- 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)
+        );
    }
 }
--- 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