Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Remove SignatureType.Caller #1015

Merged
merged 8 commits into from
Aug 24, 2018
+67 −187
Merged

Remove SignatureType.Caller #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8ce4f9c
Remove SignatureType.Caller
abandeali1 Aug 24, 2018
4f27991
Remove SigntureType.Caller from signingUtils
abandeali1 Aug 24, 2018
1932aff
Remove Trezor SignatureType
abandeali1 Aug 24, 2018
241534a
Fixed trezor personal message in client+contracts; added a test using…
hysz Aug 24, 2018
bb4d449
Test case for Trezor Model T signature
hysz Aug 24, 2018
d039a1a
Fixed linter in signatureUtils
hysz Aug 24, 2018
a27112c
SignatureType.Trezor -> SignatureType.EthSign in Signature Validator …
hysz Aug 24, 2018
0629a7d
Remove remaining Trezor references
abandeali1 Aug 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/contracts/src/2.0.0/examples/Whitelist/Whitelist.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract Whitelist is
bytes internal TX_ORIGIN_SIGNATURE;
// solhint-enable var-name-mixedcase

byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x06";
byte constant internal VALIDATOR_SIGNATURE_BYTE = "\x05";

constructor (address _exchange)
public
Expand Down
62 changes: 10 additions & 52 deletions packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,16 @@ contract MixinSignatureValidator is
)
external
{
require(
isValidSignature(
hash,
signerAddress,
signature
),
"INVALID_SIGNATURE"
);
if (signerAddress != msg.sender) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this interact with executeTransaction? Seems like currently the exchange contract can become a valid signer for anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add a require(msg.sender != this) here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue since we are using DELEGATECALL and are not otherwise making arbitrary self-calls.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this mean that if I call executeTransaction and then call preSign that I could presign orders for the Exchange contract?

I recommend to not allow adding pre signed signatures if signerAddress == msg.sender.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executeTransaction uses delegatecall, so the Exchange contract should never be msg.sender.

We've basically determined that usage of msg.sender here should be safe (this is a pretty common pattern). If we were using getCurrentContextAddress, there would definitely be extra protections.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no direct vulnerability in the 0x protocol relating to this change. A scenario where an entity creates a smart contract that has a token balance and has approved the AssetProxies could still be vulnerable if they allow forward calls to the exchange contract and the function preSign is callable. It's acknowledged that this is outside of the control of 0x and for that reason the issue is considered closed.

I still recommend caution around the assumption that no signature validation needs to be performed when msg.sender == signerAddress.

require(
isValidSignature(
hash,
signerAddress,
signature
),
"INVALID_SIGNATURE"
);
}
preSigned[hash][signerAddress] = true;
}

Expand Down Expand Up @@ -172,22 +174,6 @@ contract MixinSignatureValidator is
isValid = signerAddress == recovered;
return isValid;

// Implicitly signed by caller.
// The signer has initiated the call. In the case of non-contract
// accounts it means the transaction itself was signed.
// Example: let's say for a particular operation three signatures
// A, B and C are required. To submit the transaction, A and B can
// give a signature to C, who can then submit the transaction using
// `Caller` for his own signature. Or A and C can sign and B can
// submit using `Caller`. Having `Caller` allows this flexibility.
} else if (signatureType == SignatureType.Caller) {
require(
signature.length == 0,
"LENGTH_0_REQUIRED"
);
isValid = signerAddress == msg.sender;
return isValid;

// Signature verified by wallet contract.
// If used with an order, the maker of the order is the wallet contract.
} else if (signatureType == SignatureType.Wallet) {
Expand Down Expand Up @@ -225,34 +211,6 @@ contract MixinSignatureValidator is
} else if (signatureType == SignatureType.PreSigned) {
isValid = preSigned[hash][signerAddress];
return isValid;

// Signature from Trezor hardware wallet.
// It differs from web3.eth_sign in the encoding of message length
// (Bitcoin varint encoding vs ascii-decimal, the latter is not
// self-terminating which leads to ambiguities).
// See also:
// https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
// https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L602
// https://github.com/trezor/trezor-mcu/blob/master/firmware/crypto.c#L36
} else if (signatureType == SignatureType.Trezor) {
require(
signature.length == 65,
"LENGTH_65_REQUIRED"
);
v = uint8(signature[0]);
r = signature.readBytes32(1);
s = signature.readBytes32(33);
recovered = ecrecover(
keccak256(abi.encodePacked(
"\x19Ethereum Signed Message:\n\x20",
hash
)),
v,
r,
s
);
isValid = signerAddress == recovered;
return isValid;
}

// Anything else is illegal (We do not return false because
Expand Down
10 changes: 4 additions & 6 deletions packages/contracts/src/2.0.0/protocol/Exchange/mixins/MSignatureValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ contract MSignatureValidator is
Invalid, // 0x01
EIP712, // 0x02
EthSign, // 0x03
Caller, // 0x04
Wallet, // 0x05
Validator, // 0x06
PreSigned, // 0x07
Trezor, // 0x08
NSignatureTypes // 0x09, number of signature types. Always leave at end.
Wallet, // 0x04
Validator, // 0x05
PreSigned, // 0x06
NSignatureTypes // 0x07, number of signature types. Always leave at end.
}

/// @dev Verifies signature using logic defined by Wallet contract.
Expand Down
109 changes: 36 additions & 73 deletions packages/contracts/test/exchange/signature_validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,32 +281,6 @@ describe('MixinSignatureValidator', () => {
expect(isValidSignature).to.be.false();
});

it('should return true when SignatureType=Caller and signer is caller', async () => {
const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`);
const signatureHex = ethUtil.bufferToHex(signature);
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
orderHashHex,
signerAddress,
signatureHex,
{ from: signerAddress },
);
expect(isValidSignature).to.be.true();
});

it('should return false when SignatureType=Caller and signer is not caller', async () => {
const signature = ethUtil.toBuffer(`0x${SignatureType.Caller}`);
const signatureHex = ethUtil.bufferToHex(signature);
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
orderHashHex,
signerAddress,
signatureHex,
{ from: notSignerAddress },
);
expect(isValidSignature).to.be.false();
});

it('should return true when SignatureType=Wallet and signature is valid', async () => {
// Create EIP712 signature
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
Expand Down Expand Up @@ -440,53 +414,6 @@ describe('MixinSignatureValidator', () => {
expect(isValidSignature).to.be.false();
});

it('should return true when SignatureType=Trezor and signature is valid', async () => {
// Create Trezor signature
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor);
const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex);
const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey);
// Create 0x signature from Trezor signature
const signature = Buffer.concat([
ethUtil.toBuffer(ecSignature.v),
ecSignature.r,
ecSignature.s,
ethUtil.toBuffer(`0x${SignatureType.Trezor}`),
]);
const signatureHex = ethUtil.bufferToHex(signature);
// Validate signature
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
orderHashHex,
signerAddress,
signatureHex,
);
expect(isValidSignature).to.be.true();
});

it('should return false when SignatureType=Trezor and signature is invalid', async () => {
// Create Trezor signature
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
const orderHashWithTrezorPrefixHex = signatureUtils.addSignedMessagePrefix(orderHashHex, SignerType.Trezor);
const orderHashWithTrezorPrefixBuffer = ethUtil.toBuffer(orderHashWithTrezorPrefixHex);
const ecSignature = ethUtil.ecsign(orderHashWithTrezorPrefixBuffer, signerPrivateKey);
// Create 0x signature from Trezor signature
const signature = Buffer.concat([
ethUtil.toBuffer(ecSignature.v),
ecSignature.r,
ecSignature.s,
ethUtil.toBuffer(`0x${SignatureType.Trezor}`),
]);
const signatureHex = ethUtil.bufferToHex(signature);
// Validate signature.
// This will fail because `signerAddress` signed the message, but we're passing in `notSignerAddress`
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
orderHashHex,
notSignerAddress,
signatureHex,
);
expect(isValidSignature).to.be.false();
});

it('should return true when SignatureType=Presigned and signer has presigned hash', async () => {
// Presign hash
const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder);
Expand Down Expand Up @@ -520,6 +447,42 @@ describe('MixinSignatureValidator', () => {
);
expect(isValidSignature).to.be.false();
});

it('should return true when message was signed by a Trezor One (firmware version 1.6.2)', async () => {
// messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b
const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++'));
const signer = '0xc28b145f10f0bcf0fc000e778615f8fd73490bad';
const v = ethUtil.toBuffer('0x1c');
const r = ethUtil.toBuffer('0x7b888b596ccf87f0bacab0dcb483124973f7420f169b4824d7a12534ac1e9832');
const s = ethUtil.toBuffer('0x0c8e14f7edc01459e13965f1da56e0c23ed11e2cca932571eee1292178f90424');
const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`);
const signature = Buffer.concat([v, r, s, trezorSignatureType]);
const signatureHex = ethUtil.bufferToHex(signature);
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
messageHash,
signer,
signatureHex,
);
expect(isValidSignature).to.be.true();
});

it('should return true when message was signed by a Trezor Model T (firmware version 2.0.7)', async () => {
// messageHash translates to 0x2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b
const messageHash = ethUtil.bufferToHex(ethUtil.toBuffer('++++++++++++++++++++++++++++++++'));
const signer = '0x98ce6d9345e8ffa7d99ee0822272fae9d2c0e895';
const v = ethUtil.toBuffer('0x1c');
const r = ethUtil.toBuffer('0x423b71062c327f0ec4fe199b8da0f34185e59b4c1cb4cc23df86cac4a601fb3f');
const s = ethUtil.toBuffer('0x53810d6591b5348b7ee08ee812c874b0fdfb942c9849d59512c90e295221091f');
const trezorSignatureType = ethUtil.toBuffer(`0x${SignatureType.EthSign}`);
const signature = Buffer.concat([v, r, s, trezorSignatureType]);
const signatureHex = ethUtil.bufferToHex(signature);
const isValidSignature = await signatureValidator.publicIsValidSignature.callAsync(
messageHash,
signer,
signatureHex,
);
expect(isValidSignature).to.be.true();
});
});

describe('setSignatureValidatorApproval', () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/order-utils/CHANGELOG.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
[
{
"version": "1.0.1-rc.5",
"changes": [
{
"note": "Remove Caller and Trezor SignatureTypes",
"pr": 1015
}
]
},
{
"version": "1.0.1-rc.4",
"changes": [
Expand Down
30 changes: 2 additions & 28 deletions packages/order-utils/src/signature_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ export const signatureUtils = {
return signatureUtils.isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
}

case SignatureType.Caller:
// HACK: We currently do not "validate" the caller signature type.
// It can only be validated during Exchange contract execution.
throw new Error('Caller signature type cannot be validated off-chain');

case SignatureType.Wallet: {
const isValid = await signatureUtils.isValidWalletSignatureAsync(
provider,
Expand All @@ -82,12 +77,6 @@ export const signatureUtils = {
return signatureUtils.isValidPresignedSignatureAsync(provider, data, signerAddress);
}

case SignatureType.Trezor: {
const prefixedMessageHex = signatureUtils.addSignedMessagePrefix(data, SignerType.Trezor);
const ecSignature = signatureUtils.parseECSignature(signature);
return signatureUtils.isValidECSignature(prefixedMessageHex, ecSignature, signerAddress);
}

default:
throw new Error(`Unhandled SignatureType: ${signatureTypeIndexIfExists}`);
}
Expand Down Expand Up @@ -293,10 +282,6 @@ export const signatureUtils = {
signatureType = SignatureType.EthSign;
break;
}
case SignerType.Trezor: {
signatureType = SignatureType.Trezor;
break;
}
default:
throw new Error(`Unrecognized SignerType: ${signerType}`);
}
Expand All @@ -306,7 +291,7 @@ export const signatureUtils = {
/**
* Combines the signature proof and the Signature Type.
* @param signature The hex encoded signature proof
* @param signatureType The signature type, i.e EthSign, Trezor, Wallet etc.
* @param signatureType The signature type, i.e EthSign, Wallet etc.
* @return Hex encoded string of signature proof with Signature Type
*/
convertToSignatureWithType(signature: string, signatureType: SignatureType): string {
Expand All @@ -333,12 +318,6 @@ export const signatureUtils = {
const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
return prefixedMsgHex;
}
case SignerType.Trezor: {
const msgBuff = ethUtil.toBuffer(message);
const prefixedMsgBuff = hashTrezorPersonalMessage(msgBuff);
const prefixedMsgHex = ethUtil.bufferToHex(prefixedMsgBuff);
return prefixedMsgHex;
}
default:
throw new Error(`Unrecognized SignerType: ${signerType}`);
}
Expand All @@ -350,7 +329,7 @@ export const signatureUtils = {
*/
parseECSignature(signature: string): ECSignature {
assert.isHexString('signature', signature);
const ecSignatureTypes = [SignatureType.EthSign, SignatureType.EIP712, SignatureType.Trezor];
const ecSignatureTypes = [SignatureType.EthSign, SignatureType.EIP712];
assert.isOneOfExpectedSignatureTypes(signature, ecSignatureTypes);

// tslint:disable-next-line:custom-no-magic-numbers
Expand All @@ -361,11 +340,6 @@ export const signatureUtils = {
},
};

function hashTrezorPersonalMessage(message: Buffer): Buffer {
const prefix = ethUtil.toBuffer('\x19Ethereum Signed Message:\n' + String.fromCharCode(message.byteLength));
return ethUtil.sha3(Buffer.concat([prefix, message]));
}

function parseValidatorSignature(signature: string): ValidatorSignature {
assert.isOneOfExpectedSignatureTypes(signature, [SignatureType.Validator]);
// tslint:disable:custom-no-magic-numbers
Expand Down
23 changes: 0 additions & 23 deletions packages/order-utils/test/signature_utils_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,6 @@ describe('Signature utils', () => {
);
expect(isValidSignatureLocal).to.be.true();
});

it('should return true for a valid Trezor signature', async () => {
dataHex = '0xd0d994e31c88f33fd8a572552a70ed339de579e5ba49ee1d17cc978bbe1cdd21';
address = '0x6ecbe1db9ef729cbe972c83fb886247691fb6beb';
const trezorSignature =
'0x1ce4760660e6495b5ae6723087bea073b3a99ce98ea81fdf00c240279c010e63d05b87bc34c4d67d4776e8d5aeb023a67484f4eaf0fd353b40893e5101e845cd9908';
const isValidSignatureLocal = await signatureUtils.isValidSignatureAsync(
provider,
dataHex,
trezorSignature,
address,
);
expect(isValidSignatureLocal).to.be.true();
});
});
describe('#isValidECSignature', () => {
const signature = {
Expand Down Expand Up @@ -270,15 +256,6 @@ describe('Signature utils', () => {
r: '0xaca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d64393',
s: '0x46b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf2',
};
it('should concatenate v,r,s and append the Trezor signature type', async () => {
const expectedSignatureWithSignatureType =
'0x1baca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d6439346b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf208';
const signatureWithSignatureType = signatureUtils.convertECSignatureToSignatureHex(
ecSignature,
SignerType.Trezor,
);
expect(signatureWithSignatureType).to.equal(expectedSignatureWithSignatureType);
});
it('should concatenate v,r,s and append the EthSign signature type when SignerType is Default', async () => {
const expectedSignatureWithSignatureType =
'0x1baca7da997ad177f040240cdccf6905b71ab16b74434388c3a72f34fd25d6439346b2bac274ff29b48b3ea6e2d04c1336eaceafda3c53ab483fc3ff12fac3ebf203';
Expand Down
4 changes: 4 additions & 0 deletions packages/types/CHANGELOG.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
{
"note": "Add WalletError and ValidatorError revert reasons",
"pr": 1012
},
{
"note": "Remove Caller and Trezor SignatureTypes",
"pr": 1015
}
]
},
Expand Down
Loading