@0x/contracts-zero-ex: Prevent EP ETH balance from reducing when executin mtxs (#365)

Co-authored-by: Lawrence Forman <me@merklejerk.com>

contracts/zero-ex/CHANGELOG.json

@@ -1,4 +1,13 @@
 [
+    {
+        "version": "0.29.4",
+        "changes": [
+            {
+                "note": "Prevent EP ETH balance from reducing when executin mtxs",
+                "pr": 365
+            }
+        ]
+    },
     {
         "version": "0.29.3",
         "changes": [

contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol

@@ -78,7 +78,7 @@ contract MetaTransactionsFeature is
     /// @dev Name of this feature.
     string public constant override FEATURE_NAME = "MetaTransactions";
     /// @dev Version of this feature.
-    uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 2, 0);
+    uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 2, 1);
     /// @dev EIP712 typehash of the `MetaTransactionData` struct.
     bytes32 public immutable MTX_EIP712_TYPEHASH = keccak256(
         "MetaTransactionData("
@@ -105,6 +105,17 @@ contract MetaTransactionsFeature is
         }
     }
 
+    /// @dev Ensures that the ETH balance of `this` does not go below the
+    ///      initial ETH balance before the call (excluding ETH attached to the call).
+    modifier doesNotReduceEthBalance() {
+        uint256 initialBalance = address(this).balance - msg.value;
+        _;
+        require(
+            initialBalance <= address(this).balance,
+            "MetaTransactionsFeature/ETH_LEAK"
+        );
+    }
+
     constructor(address zeroExAddress)
         public
         FixinCommon()
@@ -140,6 +151,7 @@ contract MetaTransactionsFeature is
         payable
         override
         nonReentrant(REENTRANCY_MTX)
+        doesNotReduceEthBalance
         refundsAttachedEth
         returns (bytes memory returnResult)
     {
@@ -164,6 +176,7 @@ contract MetaTransactionsFeature is
         payable
         override
         nonReentrant(REENTRANCY_MTX)
+        doesNotReduceEthBalance
         refundsAttachedEth
         returns (bytes[] memory returnResults)
     {

contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol

@@ -46,6 +46,10 @@ contract TestMetaTransactionsTransformERC20Feature is
         payable
         returns (uint256 outputTokenAmount)
     {
+        if (msg.value == 555) {
+            tx.origin.transfer(1);
+        }
+
         if (msg.value == 666) {
             revert('FAIL');
         }

contracts/zero-ex/test/features/meta_transactions_test.ts

@@ -38,6 +38,7 @@ blockchainTests.resets('MetaTransactions feature', env => {
     let nativeOrdersFeature: TestMetaTransactionsNativeOrdersFeatureContract;
 
     const MAX_FEE_AMOUNT = new BigNumber('1e18');
+    const TRANSFORM_ERC20_ONE_WEI_VALUE = new BigNumber(555);
     const TRANSFORM_ERC20_FAILING_VALUE = new BigNumber(666);
     const TRANSFORM_ERC20_REENTER_VALUE = new BigNumber(777);
     const TRANSFORM_ERC20_BATCH_REENTER_VALUE = new BigNumber(888);
@@ -597,7 +598,7 @@ blockchainTests.resets('MetaTransactions feature', env => {
             );
         });
 
-        it('cannot reenter `executeMetaTransaction()`', async () => {
+        it('cannot reduce initial ETH balance', async () => {
             const args = getRandomTransformERC20Args();
             const mtx = getRandomMetaTransaction({
                 callData: transformERC20Feature
@@ -609,58 +610,23 @@ blockchainTests.resets('MetaTransactions feature', env => {
                         args.transformations,
                     )
                     .getABIEncodedTransactionData(),
-                value: TRANSFORM_ERC20_REENTER_VALUE,
+                value: TRANSFORM_ERC20_ONE_WEI_VALUE,
             });
-            const mtxHash = mtx.getHash();
             const signature = await mtx.getSignatureWithProviderAsync(env.provider);
             const callOpts = {
                 gasPrice: mtx.maxGasPrice,
                 value: mtx.value,
             };
-            const tx = feature.executeMetaTransaction(mtx, signature).awaitTransactionSuccessAsync(callOpts);
+            // Send pre-existing ETH to the EP.
-            return expect(tx).to.revertWith(
+            await env.web3Wrapper.awaitTransactionSuccessAsync(
-                new ZeroExRevertErrors.MetaTransactions.MetaTransactionCallFailedError(
+                await env.web3Wrapper.sendTransactionAsync({
-                    mtxHash,
+                    from: owner,
-                    undefined,
+                    to: zeroEx.address,
-                    new ZeroExRevertErrors.Common.IllegalReentrancyError(
+                    value: new BigNumber(1),
-                        feature.getSelector('executeMetaTransaction'),
+                }),
-                        REENTRANCY_FLAG_MTX,
-                    ).encode(),
-                ),
             );
-        });
-
-        it('cannot reenter `batchExecuteMetaTransactions()`', async () => {
-            const args = getRandomTransformERC20Args();
-            const mtx = getRandomMetaTransaction({
-                callData: transformERC20Feature
-                    .transformERC20(
-                        args.inputToken,
-                        args.outputToken,
-                        args.inputTokenAmount,
-                        args.minOutputTokenAmount,
-                        args.transformations,
-                    )
-                    .getABIEncodedTransactionData(),
-                value: TRANSFORM_ERC20_BATCH_REENTER_VALUE,
-            });
-            const mtxHash = mtx.getHash();
-            const signature = await mtx.getSignatureWithProviderAsync(env.provider);
-            const callOpts = {
-                gasPrice: mtx.maxGasPrice,
-                value: mtx.value,
-            };
             const tx = feature.executeMetaTransaction(mtx, signature).awaitTransactionSuccessAsync(callOpts);
-            return expect(tx).to.revertWith(
+            return expect(tx).to.revertWith('MetaTransactionsFeature/ETH_LEAK');
-                new ZeroExRevertErrors.MetaTransactions.MetaTransactionCallFailedError(
-                    mtxHash,
-                    undefined,
-                    new ZeroExRevertErrors.Common.IllegalReentrancyError(
-                        feature.getSelector('batchExecuteMetaTransactions'),
-                        REENTRANCY_FLAG_MTX,
-                    ).encode(),
-                ),
-            );
         });
     });
 
@@ -817,6 +783,37 @@ blockchainTests.resets('MetaTransactions feature', env => {
                 ),
             );
         });
+
+        it('cannot reduce initial ETH balance', async () => {
+            const args = getRandomTransformERC20Args();
+            const mtx = getRandomMetaTransaction({
+                callData: transformERC20Feature
+                    .transformERC20(
+                        args.inputToken,
+                        args.outputToken,
+                        args.inputTokenAmount,
+                        args.minOutputTokenAmount,
+                        args.transformations,
+                    )
+                    .getABIEncodedTransactionData(),
+                value: TRANSFORM_ERC20_ONE_WEI_VALUE,
+            });
+            const signature = await mtx.getSignatureWithProviderAsync(env.provider);
+            const callOpts = {
+                gasPrice: mtx.maxGasPrice,
+                value: mtx.value,
+            };
+            // Send pre-existing ETH to the EP.
+            await env.web3Wrapper.awaitTransactionSuccessAsync(
+                await env.web3Wrapper.sendTransactionAsync({
+                    from: owner,
+                    to: zeroEx.address,
+                    value: new BigNumber(1),
+                }),
+            );
+            const tx = feature.batchExecuteMetaTransactions([mtx], [signature]).awaitTransactionSuccessAsync(callOpts);
+            return expect(tx).to.revertWith('MetaTransactionsFeature/ETH_LEAK');
+        });
     });
 
     describe('getMetaTransactionExecutedBlock()', () => {