diff options
6 files changed, 198 insertions, 15 deletions
diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol index fd655e757..6bfbb5107 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol @@ -17,7 +17,6 @@ */ pragma solidity 0.4.24; -pragma experimental "v0.5.0"; import "../../utils/LibBytes/LibBytes.sol"; import "./mixins/MSignatureValidator.sol"; @@ -192,7 +191,11 @@ contract MixinSignatureValidator is // Signature verified by wallet contract. // If used with an order, the maker of the order is the wallet contract. } else if (signatureType == SignatureType.Wallet) { - isValid = IWallet(signerAddress).isValidSignature(hash, signature); + isValid = isValidWalletSignature( + hash, + signerAddress, + signature + ); return isValid; // Signature verified by validator contract. @@ -210,7 +213,8 @@ contract MixinSignatureValidator is if (!allowedValidators[signerAddress][validatorAddress]) { return false; } - isValid = IValidator(validatorAddress).isValidSignature( + isValid = isValidValidatorSignature( + validatorAddress, hash, signerAddress, signature @@ -258,4 +262,78 @@ contract MixinSignatureValidator is // signature was invalid.) revert("SIGNATURE_UNSUPPORTED"); } + + /// @dev Verifies signature using logic defined by Wallet contract. + /// @param hash Any 32 byte hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own signature verification method. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if signature is valid for given wallet.. + function isValidWalletSignature( + bytes32 hash, + address walletAddress, + bytes signature + ) + internal + view + returns (bool isValid) + { + bytes memory calldata = abi.encodeWithSelector( + IWallet(walletAddress).isValidSignature.selector, + hash, + signature + ); + assembly { + let cdStart := add(calldata, 32) + let success := staticcall( + gas, // forward all gas + walletAddress, // address of Wallet contract + cdStart, // pointer to start of input + mload(calldata), // length of input + cdStart, // write input over output + 32 // output size is 32 bytes + ) + // Signature is valid if call did not revert and returned true + isValid := and(success, mload(cdStart)) + } + return isValid; + } + + /// @dev Verifies signature using logic defined by Validator contract. + /// @param validatorAddress Address of validator contract. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidValidatorSignature( + address validatorAddress, + bytes32 hash, + address signerAddress, + bytes signature + ) + internal + view + returns (bool isValid) + { + bytes memory calldata = abi.encodeWithSelector( + IValidator(signerAddress).isValidSignature.selector, + hash, + signerAddress, + signature + ); + assembly { + let cdStart := add(calldata, 32) + let success := staticcall( + gas, // forward all gas + validatorAddress, // address of Validator contract + cdStart, // pointer to start of input + mload(calldata), // length of input + cdStart, // write input over output + 32 // output size is 32 bytes + ) + // Signature is valid if call did not revert and returned true + isValid := and(success, mload(cdStart)) + } + return isValid; + } } diff --git a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol index f14f2ba00..75fe9ec46 100644 --- a/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol @@ -43,4 +43,35 @@ contract MSignatureValidator is Trezor, // 0x08 NSignatureTypes // 0x09, number of signature types. Always leave at end. } + + /// @dev Verifies signature using logic defined by Wallet contract. + /// @param hash Any 32 byte hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own signature verification method. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidWalletSignature( + bytes32 hash, + address walletAddress, + bytes signature + ) + internal + view + returns (bool isValid); + + /// @dev Verifies signature using logic defined by Validator contract. + /// @param validatorAddress Address of validator contract. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidValidatorSignature( + address validatorAddress, + bytes32 hash, + address signerAddress, + bytes signature + ) + internal + view + returns (bool isValid); } diff --git a/packages/contracts/src/2.0.0/test/TestSignatureValidator/TestSignatureValidator.sol b/packages/contracts/src/2.0.0/test/TestSignatureValidator/TestSignatureValidator.sol index 3845b7b9e..e1a610469 100644 --- a/packages/contracts/src/2.0.0/test/TestSignatureValidator/TestSignatureValidator.sol +++ b/packages/contracts/src/2.0.0/test/TestSignatureValidator/TestSignatureValidator.sol @@ -17,7 +17,6 @@ */ pragma solidity 0.4.24; -pragma experimental "v0.5.0"; import "../../protocol/Exchange/MixinSignatureValidator.sol"; import "../../protocol/Exchange/MixinTransactions.sol"; diff --git a/packages/contracts/src/2.0.0/test/TestStaticCall/TestStaticCall.sol b/packages/contracts/src/2.0.0/test/TestStaticCall/TestStaticCall.sol index be24e2f37..5a9581f51 100644 --- a/packages/contracts/src/2.0.0/test/TestStaticCall/TestStaticCall.sol +++ b/packages/contracts/src/2.0.0/test/TestStaticCall/TestStaticCall.sol @@ -18,6 +18,8 @@ pragma solidity 0.4.24; +import "../../tokens/ERC20Token/IERC20Token.sol"; + contract TestStaticCall { @@ -55,6 +57,20 @@ contract TestStaticCall { return true; } + /// @dev Approves an ERC20 token to spend tokens from this address. + /// @param token Address of ERC20 token. + /// @param spender Address that will spend tokens. + /// @param value Amount of tokens spender is approved to spend. + function approveERC20( + address token, + address spender, + uint256 value + ) + external + { + IERC20Token(token).approve(spender, value); + } + /// @dev Increments state variable. function updateState() internal diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index bc2bad749..d3f9b95af 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -1,6 +1,6 @@ import { BlockchainLifecycle } from '@0xproject/dev-utils'; import { assetDataUtils, orderHashUtils } from '@0xproject/order-utils'; -import { RevertReason, SignedOrder } from '@0xproject/types'; +import { RevertReason, SignatureType, SignedOrder } from '@0xproject/types'; import { BigNumber } from '@0xproject/utils'; import { Web3Wrapper } from '@0xproject/web3-wrapper'; import * as chai from 'chai'; @@ -14,6 +14,7 @@ import { DummyNoReturnERC20TokenContract } from '../../generated_contract_wrappe import { ERC20ProxyContract } from '../../generated_contract_wrappers/erc20_proxy'; import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_proxy'; import { ExchangeCancelEventArgs, ExchangeContract } from '../../generated_contract_wrappers/exchange'; +import { TestStaticCallContract } from '../../generated_contract_wrappers/test_static_call'; import { artifacts } from '../utils/artifacts'; import { expectTransactionFailedAsync } from '../utils/assertions'; import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp'; @@ -44,6 +45,8 @@ describe('Exchange core', () => { let exchange: ExchangeContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let maliciousWallet: TestStaticCallContract; + let maliciousValidator: TestStaticCallContract; let signedOrder: SignedOrder; let erc20Balances: ERC20BalancesByOwner; @@ -109,6 +112,12 @@ describe('Exchange core', () => { constants.AWAIT_TRANSACTION_MINED_MS, ); + maliciousWallet = maliciousValidator = await TestStaticCallContract.deployFrom0xArtifactAsync( + artifacts.TestStaticCall, + provider, + txDefaults, + ); + defaultMakerAssetAddress = erc20TokenA.address; defaultTakerAssetAddress = erc20TokenB.address; @@ -161,6 +170,54 @@ describe('Exchange core', () => { RevertReason.OrderUnfillable, ); }); + + it('should revert if `isValidSignature` tries to update state when SignatureType=Wallet', async () => { + const maliciousMakerAddress = maliciousWallet.address; + await web3Wrapper.awaitTransactionSuccessAsync( + await erc20TokenA.setBalance.sendTransactionAsync( + maliciousMakerAddress, + constants.INITIAL_ERC20_BALANCE, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + await web3Wrapper.awaitTransactionSuccessAsync( + await maliciousWallet.approveERC20.sendTransactionAsync( + erc20TokenA.address, + erc20Proxy.address, + constants.INITIAL_ERC20_ALLOWANCE, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAddress: maliciousMakerAddress, + makerFee: constants.ZERO_AMOUNT, + }); + signedOrder.signature = `0x0${SignatureType.Wallet}`; + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.InvalidOrderSignature, + ); + }); + + it('should revert if `isValidSignature` tries to update state when SignatureType=Validator', async () => { + const isApproved = true; + await web3Wrapper.awaitTransactionSuccessAsync( + await exchange.setSignatureValidatorApproval.sendTransactionAsync( + maliciousValidator.address, + isApproved, + { from: makerAddress }, + ), + constants.AWAIT_TRANSACTION_MINED_MS, + ); + signedOrder = await orderFactory.newSignedOrderAsync({ + makerAddress: maliciousValidator.address, + }); + signedOrder.signature = `${maliciousValidator.address}0${SignatureType.Validator}`; + await expectTransactionFailedAsync( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress), + RevertReason.InvalidOrderSignature, + ); + }); }); describe('Testing exchange of ERC20 tokens with no return values', () => { diff --git a/packages/contracts/test/exchange/signature_validator.ts b/packages/contracts/test/exchange/signature_validator.ts index 0e2967bc7..75656d992 100644 --- a/packages/contracts/test/exchange/signature_validator.ts +++ b/packages/contracts/test/exchange/signature_validator.ts @@ -352,7 +352,7 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); - it('should not allow `isValidSignature` to update state when SignatureType=Wallet', async () => { + it('should return false when `isValidSignature` attempts to update state and not allow state to be updated when SignatureType=Wallet', async () => { // Create EIP712 signature const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); const orderHashBuffer = ethUtil.toBuffer(orderHashHex); @@ -366,13 +366,12 @@ describe('MixinSignatureValidator', () => { ]); const signatureHex = ethUtil.bufferToHex(signature); // Validate signature - await expectContractCallFailedWithoutReasonAsync( - signatureValidator.publicIsValidSignature.callAsync( - orderHashHex, - maliciousWallet.address, - signatureHex, - ), + const isValid = await signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + maliciousWallet.address, + signatureHex, ); + expect(isValid).to.be.equal(false); }); it('should return true when SignatureType=Validator, signature is valid and validator is approved', async () => { @@ -405,15 +404,18 @@ describe('MixinSignatureValidator', () => { expect(isValidSignature).to.be.false(); }); - it('should not allow `isValidSignature` to update state when SignatureType=Validator', async () => { + it('should return false when `isValidSignature` attempts to update state and not allow state to be updated when SignatureType=Validator', async () => { const validatorAddress = ethUtil.toBuffer(`${maliciousValidator.address}`); const signatureType = ethUtil.toBuffer(`0x${SignatureType.Validator}`); const signature = Buffer.concat([validatorAddress, signatureType]); const signatureHex = ethUtil.bufferToHex(signature); const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - await expectContractCallFailedWithoutReasonAsync( - signatureValidator.publicIsValidSignature.callAsync(orderHashHex, signerAddress, signatureHex), + const isValid = await signatureValidator.publicIsValidSignature.callAsync( + orderHashHex, + maliciousValidator.address, + signatureHex, ); + expect(isValid).to.be.equal(false); }); it('should return false when SignatureType=Validator, signature is valid and validator is not approved', async () => { // Set approval of signature validator to false |