Remove pragma experimental v0.5.0 and use staticcall is assembly

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