diff --git a/contracts/integrations/test/exchange-proxy/mtx_test.ts b/contracts/integrations/test/exchange-proxy/mtx_test.ts index c1844e84b3..f6c0f60b4e 100644 --- a/contracts/integrations/test/exchange-proxy/mtx_test.ts +++ b/contracts/integrations/test/exchange-proxy/mtx_test.ts @@ -6,6 +6,7 @@ import { artifacts as exchangeProxyArtifacts, IZeroExContract, LogMetadataTransformerContract, + Signature, signCallData, } from '@0x/contracts-zero-ex'; import { migrateOnceAsync } from '@0x/migrations'; @@ -25,6 +26,15 @@ import * as ethjs from 'ethereumjs-util'; const { MAX_UINT256, NULL_ADDRESS, NULL_BYTES, NULL_BYTES32, ZERO_AMOUNT } = constants; +function sigstruct(signature: string): Signature { + return { + v: parseInt(hexUtils.slice(signature, 0, 1), 16), + signatureType: parseInt(hexUtils.slice(signature, 65, 66), 16), + r: hexUtils.slice(signature, 1, 33), + s: hexUtils.slice(signature, 33, 65), + }; +} + blockchainTests.resets('exchange proxy - meta-transactions', env => { const quoteSignerKey = hexUtils.random(); const quoteSigner = hexUtils.toHex(ethjs.privateToAddress(ethjs.toBuffer(quoteSignerKey))); @@ -240,7 +250,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const mtx = await createMetaTransactionAsync(signedSwapData, _protocolFee, 0); const relayerEthBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(relayer); const receipt = await zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); const relayerEthRefund = relayerEthBalanceBefore .minus(await env.web3Wrapper.getBalanceInWeiAsync(relayer)) @@ -276,7 +286,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const mtx = await createMetaTransactionAsync(signedSwapData, _protocolFee); const relayerEthBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(relayer); const receipt = await zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); const relayerEthRefund = relayerEthBalanceBefore .minus(await env.web3Wrapper.getBalanceInWeiAsync(relayer)) @@ -311,7 +321,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const mtx = await createMetaTransactionAsync(signedSwapData, _protocolFee); const relayerEthBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(relayer); const receipt = await zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); const relayerEthRefund = relayerEthBalanceBefore .minus(await env.web3Wrapper.getBalanceInWeiAsync(relayer)) @@ -348,7 +358,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const mtx = await createMetaTransactionAsync(signedSwapData, _protocolFee, 0); const relayerEthBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(relayer); const receipt = await zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); const relayerEthRefund = relayerEthBalanceBefore .minus(await env.web3Wrapper.getBalanceInWeiAsync(relayer)) @@ -385,7 +395,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const relayerEthBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(relayer); await zeroEx.setQuoteSigner(NULL_ADDRESS).awaitTransactionSuccessAsync({ from: owner }); const receipt = await zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); const relayerEthRefund = relayerEthBalanceBefore .minus(await env.web3Wrapper.getBalanceInWeiAsync(relayer)) @@ -419,7 +429,7 @@ blockchainTests.resets('exchange proxy - meta-transactions', env => { const _protocolFee = protocolFee.times(GAS_PRICE).times(swap.orders.length + 1); // Pay a little more fee than needed. const mtx = await createMetaTransactionAsync(callData, _protocolFee, 0); const tx = zeroEx - .executeMetaTransaction(mtx, mtx.signature) + .executeMetaTransaction(mtx, sigstruct(mtx.signature)) .awaitTransactionSuccessAsync({ from: relayer, value: mtx.value, gasPrice: GAS_PRICE }); return expect(tx).to.revertWith(new ZeroExRevertErrors.MetaTransactions.MetaTransactionCallFailedError()); }); diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index 3fc6efe662..bc71ce44e2 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -21,6 +21,10 @@ { "note": "Add a gas limit to first `LibTokenSpender` and `UniswapFeature` transfer", "pr": 38 + }, + { + "note": "Convert metatransactions to use `LibSignature`", + "pr": 31 } ] }, diff --git a/contracts/zero-ex/contracts/src/features/IMetaTransactionsFeature.sol b/contracts/zero-ex/contracts/src/features/IMetaTransactionsFeature.sol index ceb14e2d36..caaf9ec4dc 100644 --- a/contracts/zero-ex/contracts/src/features/IMetaTransactionsFeature.sol +++ b/contracts/zero-ex/contracts/src/features/IMetaTransactionsFeature.sol @@ -20,11 +20,10 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; import "@0x/contracts-erc20/contracts/src/v06/IERC20TokenV06.sol"; - +import "./libs/LibSignature.sol"; /// @dev Meta-transactions feature. interface IMetaTransactionsFeature { - /// @dev Describes an exchange proxy meta transaction. struct MetaTransactionData { // Signer of meta-transaction. On whose behalf to execute the MTX. @@ -68,7 +67,7 @@ interface IMetaTransactionsFeature { /// @return returnResult The ABI-encoded result of the underlying call. function executeMetaTransaction( MetaTransactionData calldata mtx, - bytes calldata signature + LibSignature.Signature calldata signature ) external payable @@ -80,7 +79,7 @@ interface IMetaTransactionsFeature { /// @return returnResults The ABI-encoded results of the underlying calls. function batchExecuteMetaTransactions( MetaTransactionData[] calldata mtxs, - bytes[] calldata signatures + LibSignature.Signature[] calldata signatures ) external payable @@ -88,14 +87,14 @@ interface IMetaTransactionsFeature { /// @dev Execute a meta-transaction via `sender`. Privileged variant. /// Only callable from within. - /// @param sender Who is executing the meta-transaction.. + /// @param sender Who is executing the meta-transaction. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. /// @return returnResult The ABI-encoded result of the underlying call. function _executeMetaTransaction( address sender, - MetaTransactionData calldata mtx, - bytes calldata signature + MetaTransactionData memory mtx, + LibSignature.Signature memory signature ) external payable diff --git a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol index 81070d4bbb..c51e8fdb43 100644 --- a/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol +++ b/contracts/zero-ex/contracts/src/features/MetaTransactionsFeature.sol @@ -32,6 +32,7 @@ import "../storage/LibMetaTransactionsStorage.sol"; import "./libs/LibSignedCallData.sol"; import "./IMetaTransactionsFeature.sol"; import "./ITransformERC20Feature.sol"; +import "./libs/LibSignature.sol"; import "./ISignatureValidatorFeature.sol"; import "./IFeature.sol"; @@ -47,8 +48,8 @@ contract MetaTransactionsFeature is using LibBytesV06 for bytes; using LibRichErrorsV06 for bytes; - /// @dev Intermediate state vars used by `_executeMetaTransactionPrivate()` - /// to avoid stack overflows. + + /// @dev Describes the state of a meta transaction. struct ExecuteState { // Sender of the meta-transaction. address sender; @@ -57,7 +58,7 @@ contract MetaTransactionsFeature is // The meta-transaction data. MetaTransactionData mtx; // The meta-transaction signature (by `mtx.signer`). - bytes signature; + LibSignature.Signature signature; // The selector of the function being called. bytes4 selector; // The ETH balance of this contract before performing the call. @@ -136,7 +137,7 @@ contract MetaTransactionsFeature is /// @return returnResult The ABI-encoded result of the underlying call. function executeMetaTransaction( MetaTransactionData memory mtx, - bytes memory signature + LibSignature.Signature memory signature ) public payable @@ -145,11 +146,13 @@ contract MetaTransactionsFeature is refundsAttachedEth returns (bytes memory returnResult) { - returnResult = _executeMetaTransactionPrivate( - msg.sender, - mtx, - signature - ); + ExecuteState memory state; + state.sender = msg.sender; + state.mtx = mtx; + state.hash = getMetaTransactionHash(mtx); + state.signature = signature; + + returnResult = _executeMetaTransactionPrivate(state); } /// @dev Execute multiple meta-transactions. @@ -158,7 +161,7 @@ contract MetaTransactionsFeature is /// @return returnResults The ABI-encoded results of the underlying calls. function batchExecuteMetaTransactions( MetaTransactionData[] memory mtxs, - bytes[] memory signatures + LibSignature.Signature[] memory signatures ) public payable @@ -175,11 +178,13 @@ contract MetaTransactionsFeature is } returnResults = new bytes[](mtxs.length); for (uint256 i = 0; i < mtxs.length; ++i) { - returnResults[i] = _executeMetaTransactionPrivate( - msg.sender, - mtxs[i], - signatures[i] - ); + ExecuteState memory state; + state.sender = msg.sender; + state.mtx = mtxs[i]; + state.hash = getMetaTransactionHash(mtxs[i]); + state.signature = signatures[i]; + + returnResults[i] = _executeMetaTransactionPrivate(state); } } @@ -192,7 +197,7 @@ contract MetaTransactionsFeature is function _executeMetaTransaction( address sender, MetaTransactionData memory mtx, - bytes memory signature + LibSignature.Signature memory signature ) public payable @@ -200,7 +205,13 @@ contract MetaTransactionsFeature is onlySelf returns (bytes memory returnResult) { - return _executeMetaTransactionPrivate(sender, mtx, signature); + ExecuteState memory state; + state.sender = sender; + state.mtx = mtx; + state.hash = getMetaTransactionHash(mtx); + state.signature = signature; + + return _executeMetaTransactionPrivate(state); } /// @dev Get the block at which a meta-transaction has been executed. @@ -252,24 +263,13 @@ contract MetaTransactionsFeature is } /// @dev Execute a meta-transaction by `sender`. Low-level, hidden variant. - /// @param sender Who is executing the meta-transaction.. - /// @param mtx The meta-transaction. - /// @param signature The signature by `mtx.signer`. + /// @param state The `ExecuteState` for this metatransaction, with `sender`, + /// `hash`, `mtx`, and `signature` fields filled. /// @return returnResult The ABI-encoded result of the underlying call. - function _executeMetaTransactionPrivate( - address sender, - MetaTransactionData memory mtx, - bytes memory signature - ) + function _executeMetaTransactionPrivate(ExecuteState memory state) private returns (bytes memory returnResult) { - ExecuteState memory state; - state.sender = sender; - state.hash = getMetaTransactionHash(mtx); - state.mtx = mtx; - state.signature = signature; - _validateMetaTransaction(state); // Mark the transaction executed by storing the block at which it was executed. @@ -279,17 +279,17 @@ contract MetaTransactionsFeature is .mtxHashToExecutedBlockNumber[state.hash] = block.number; // Pay the fee to the sender. - if (mtx.feeAmount > 0) { + if (state.mtx.feeAmount > 0) { _transferERC20Tokens( - mtx.feeToken, - mtx.signer, - sender, - mtx.feeAmount + state.mtx.feeToken, + state.mtx.signer, + state.sender, + state.mtx.feeAmount ); } // Execute the call based on the selector. - state.selector = mtx.callData.readBytes4(0); + state.selector = state.mtx.callData.readBytes4(0); if (state.selector == ITransformERC20Feature.transformERC20.selector) { returnResult = _executeTransformERC20Call(state); } else { @@ -300,8 +300,8 @@ contract MetaTransactionsFeature is emit MetaTransactionExecuted( state.hash, state.selector, - mtx.signer, - mtx.sender + state.mtx.signer, + state.mtx.sender ); } @@ -348,18 +348,17 @@ contract MetaTransactionsFeature is state.mtx.value ).rrevert(); } - // Must be signed by signer. - try - ISignatureValidatorFeature(address(this)) - .validateHashSignature(state.hash, state.mtx.signer, state.signature) - {} - catch (bytes memory err) { - LibMetaTransactionsRichErrors - .MetaTransactionInvalidSignatureError( - state.hash, - state.signature, - err - ).rrevert(); + + if (LibSignature.getSignerOfHash(state.hash, state.signature) != + state.mtx.signer) { + LibSignatureRichErrors.SignatureValidationError( + LibSignatureRichErrors.SignatureValidationErrorCodes.WRONG_SIGNER, + state.hash, + state.mtx.signer, + // TODO: Remove this field from SignatureValidationError + // when rich reverts are part of the protocol repo. + "" + ).rrevert(); } // Transaction must not have been already executed. state.executedBlockNumber = LibMetaTransactionsStorage diff --git a/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol b/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol index 3b794608a7..31fe821df7 100644 --- a/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol +++ b/contracts/zero-ex/contracts/test/TestMetaTransactionsTransformERC20Feature.sol @@ -52,6 +52,8 @@ contract TestMetaTransactionsTransformERC20Feature is } if (msg.value == 777) { + LibSignature.Signature memory signature; + // Try to reenter `executeMetaTransaction()` IMetaTransactionsFeature(address(this)).executeMetaTransaction( IMetaTransactionsFeature.MetaTransactionData({ @@ -66,7 +68,7 @@ contract TestMetaTransactionsTransformERC20Feature is feeToken: IERC20TokenV06(0), feeAmount: 0 }), - "" + signature ); } @@ -74,7 +76,7 @@ contract TestMetaTransactionsTransformERC20Feature is // Try to reenter `batchExecuteMetaTransactions()` IMetaTransactionsFeature.MetaTransactionData[] memory mtxs = new IMetaTransactionsFeature.MetaTransactionData[](1); - bytes[] memory signatures = new bytes[](1); + LibSignature.Signature[] memory signatures = new LibSignature.Signature[](1); mtxs[0] = IMetaTransactionsFeature.MetaTransactionData({ signer: address(0), sender: address(0), @@ -87,7 +89,6 @@ contract TestMetaTransactionsTransformERC20Feature is feeToken: IERC20TokenV06(0), feeAmount: 0 }); - signatures[0] = ""; IMetaTransactionsFeature(address(this)).batchExecuteMetaTransactions( mtxs, signatures diff --git a/contracts/zero-ex/test/features/meta_transactions_test.ts b/contracts/zero-ex/test/features/meta_transactions_test.ts index 2ffb3127c4..8a0e138a37 100644 --- a/contracts/zero-ex/test/features/meta_transactions_test.ts +++ b/contracts/zero-ex/test/features/meta_transactions_test.ts @@ -11,6 +11,7 @@ import { ExchangeProxyMetaTransaction } from '@0x/types'; import { BigNumber, hexUtils, StringRevertError, ZeroExRevertErrors } from '@0x/utils'; import * as _ from 'lodash'; +import { Signature } from '../../src/signature_utils'; import { generateCallDataSignature, signCallData } from '../../src/signed_call_data'; import { IZeroExContract, MetaTransactionsFeatureContract } from '../../src/wrappers'; import { artifacts } from '../artifacts'; @@ -71,6 +72,15 @@ blockchainTests.resets('MetaTransactions feature', env => { ); }); + function sigstruct(signature: string): Signature { + return { + v: parseInt(hexUtils.slice(signature, 0, 1), 16), + signatureType: parseInt(hexUtils.slice(signature, 65, 66), 16), + r: hexUtils.slice(signature, 1, 33), + s: hexUtils.slice(signature, 33, 65), + }; + } + function getRandomMetaTransaction( fields: Partial = {}, ): ExchangeProxyMetaTransaction { @@ -93,11 +103,13 @@ blockchainTests.resets('MetaTransactions feature', env => { }; } - async function signMetaTransactionAsync(mtx: ExchangeProxyMetaTransaction, signer?: string): Promise { - return signatureUtils.ecSignHashAsync( - env.provider, - getExchangeProxyMetaTransactionHash(mtx), - signer || mtx.signer, + async function signMetaTransactionAsync(mtx: ExchangeProxyMetaTransaction, signer?: string): Promise { + return sigstruct( + await signatureUtils.ecSignHashAsync( + env.provider, + getExchangeProxyMetaTransactionHash(mtx), + signer || mtx.signer, + ), ); } @@ -461,15 +473,11 @@ blockchainTests.resets('MetaTransactions feature', env => { }; const tx = feature.executeMetaTransaction(mtx, signature).awaitTransactionSuccessAsync(callOpts); return expect(tx).to.revertWith( - new ZeroExRevertErrors.MetaTransactions.MetaTransactionInvalidSignatureError( + new ZeroExRevertErrors.SignatureValidator.SignatureValidationError( + ZeroExRevertErrors.SignatureValidator.SignatureValidationErrorCodes.WrongSigner, mtxHash, - signature, - new ZeroExRevertErrors.SignatureValidator.SignatureValidationError( - ZeroExRevertErrors.SignatureValidator.SignatureValidationErrorCodes.WrongSigner, - mtxHash, - signers[0], - signature, - ).encode(), + signers[0], + '0x', ), ); });