Skip to content

Commit

Permalink
Fixed splitSignature computing wrong v for BytesLike (#700).
Browse files Browse the repository at this point in the history
  • Loading branch information
@ricmoo
ricmoo committed Jan 11, 2020
1 parent c846649 commit 4151c0e
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 84 deletions.
107 changes: 46 additions & 61 deletions packages/bytes/src.ts/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,18 +335,13 @@ export function splitSignature(signature: SignatureLike): Signature {
logger.throwArgumentError("invalid signature string; must be 65 bytes", "signature", signature);
}

// Get the r and s
// Get the r, s and v
result.r = hexlify(bytes.slice(0, 32));
result.s = hexlify(bytes.slice(32, 64));

// Reduce v to the canonical 27 or 28
result.v = bytes[64];
if (result.v !== 27 && result.v !== 28) {
result.v = 27 + (result.v % 2);
}

// Compute recoveryParam from v
result.recoveryParam = (result.v - 27);
result.recoveryParam = 1 - (result.v % 2);

// Compute _vs from recoveryParam and s
if (result.recoveryParam) { bytes[32] |= 0x80; }
Expand All @@ -359,86 +354,76 @@ export function splitSignature(signature: SignatureLike): Signature {
result.recoveryParam = signature.recoveryParam;
result._vs = signature._vs;

// Normalize v into a canonical 27 or 28
if (result.v != null && !(result.v == 27 || result.v == 28)) {
result.v = 27 + (result.v % 2);
}

// Populate a missing v or recoveryParam if possible
if (result.recoveryParam == null && result.v != null) {
result.recoveryParam = 1 - (result.v % 2);
} else if (result.recoveryParam != null && result.v == null) {
result.v = 27 + result.recoveryParam;
} else if (result.recoveryParam != null && result.v != null) {
if (result.v !== 27 + result.recoveryParam) {
logger.throwArgumentError("signature v mismatch recoveryParam", "signature", signature);
}
}

// Make sure r and s are padded properly
if (result.r != null) { result.r = hexZeroPad(result.r, 32); }
if (result.s != null) { result.s = hexZeroPad(result.s, 32); }

// If the _vs is available, use it to populate missing s, v and recoveryParam
// and verify non-missing s, v and recoveryParam
if (result._vs != null) {
result._vs = hexZeroPad(result._vs, 32);
if (result._vs.length > 66) {
logger.throwArgumentError("signature _vs overflow", "signature", signature);
}

const vs = arrayify(result._vs);
const vs = zeroPad(arrayify(result._vs), 32);
result._vs = hexlify(vs);

// Set or check the recid
const recoveryParam = ((vs[0] >= 128) ? 1: 0);
const v = 27 + result.recoveryParam;
if (result.recoveryParam == null) {
result.recoveryParam = recoveryParam;
} else if (result.recoveryParam !== recoveryParam) {
logger.throwArgumentError("signature recoveryParam mismatch _vs", "signature", signature);
}

// Use _vs to compute s
// Set or check the s
vs[0] &= 0x7f;
const s = hexlify(vs);

// Check _vs aggress with other parameters

if (result.s == null) {
result.s = s;
} else if (result.s !== s) {
logger.throwArgumentError("signature v mismatch _vs", "signature", signature);
}
}

// Use recid and v to populate each other
if (result.recoveryParam == null) {
if (result.v == null) {
result.v = v;
} else if (result.v !== v) {
logger.throwArgumentError("signature v mismatch _vs", "signature", signature);
logger.throwArgumentError("signature missing v and recoveryParam", "signature", signature);
} else {
result.recoveryParam = 1 - (result.v % 2);
}

if (recoveryParam == null) {
result.recoveryParam = recoveryParam;
} else if (result.recoveryParam !== recoveryParam) {
logger.throwArgumentError("signature recoveryParam mismatch _vs", "signature", signature);
} else {
if (result.v == null) {
result.v = 27 + result.recoveryParam;
} else if (result.recoveryParam !== (1 - (result.v % 2))) {
logger.throwArgumentError("signature recoveryParam mismatch v", "signature", signature);
}
}

// After all populating, both v and recoveryParam are still missing...
if (result.v == null && result.recoveryParam == null) {
logger.throwArgumentError("signature requires at least one of recoveryParam, v or _vs", "signature", signature);
if (result.r == null || !isHexString(result.r)) {
logger.throwArgumentError("signature missing or invalid r", "signature", signature);
} else {
result.r = hexZeroPad(result.r, 32);
}

// Check for canonical v
if (result.v !== 27 && result.v !== 28) {
logger.throwArgumentError("signature v not canonical", "signature", signature);
if (result.s == null || !isHexString(result.s)) {
logger.throwArgumentError("signature missing or invalid s", "signature", signature);
} else {
result.s = hexZeroPad(result.s, 32);
}

// Check that r and s are in range
if (result.r.length > 66 || result.s.length > 66) {
logger.throwArgumentError("signature overflow r or s", "signature", signature);
const vs = arrayify(result.s);
if (vs[0] >= 128) {
logger.throwArgumentError("signature s out of range", "signature", signature);
}
if (result.recoveryParam) { vs[0] |= 0x80; }
const _vs = hexlify(vs);

if (result._vs == null) {
const vs = arrayify(result.s);
if (vs[0] >= 128) {
logger.throwArgumentError("signature s out of range", "signature", signature);
if (result._vs) {
if (!isHexString(result._vs)) {
logger.throwArgumentError("signature invalid _vs", "signature", signature);
}
if (result.recoveryParam) { vs[0] |= 0x80; }
result._vs = hexlify(vs);
result._vs = hexZeroPad(result._vs, 32);
}

// Set or check the _vs
if (result._vs == null) {
result._vs = _vs;
} else if (result._vs !== _vs) {
logger.throwArgumentError("signature _vs mismatch v and s", "signature", signature);
}
}

Expand Down
19 changes: 19 additions & 0 deletions packages/testcases/src.ts/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,25 @@ export module TestCase {
finney_format?: string,
satoshi_format?: string
}

export type SignedTransaction = {
name: string;
accountAddress: string;
privateKey: string;

signedTransaction: string
unsignedTransaction: string;

signedTransactionChainId5: string
unsignedTransactionChainId5: string;

nonce: number;
gasLimit: string;
gasPrice: string;
to: string;
value: string;
data: string;
};
}

export function saveTests(tag: string, data: any) {
Expand Down
34 changes: 34 additions & 0 deletions packages/tests/src.ts/test-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,3 +487,37 @@ describe("Test nameprep", function() {
});
});
});

describe("Test Signature Manipulation", function() {
const tests: Array<TestCase.SignedTransaction> = loadTests("transactions");
tests.forEach((test) => {
it("autofills partial signatures - " + test.name, function() {
const address = ethers.utils.getAddress(test.accountAddress);
const hash = ethers.utils.keccak256(test.unsignedTransaction);
const data = ethers.utils.RLP.decode(test.signedTransaction);
const s = data.pop(), r = data.pop(), v = parseInt(data.pop().substring(2), 16);
const sig = ethers.utils.splitSignature({ r: r, s: s, v: v });

{
const addr = ethers.utils.recoverAddress(hash, {
r: r, s: s, v: v
});
assert.equal(addr, address, "Using r, s and v");
}

{
const addr = ethers.utils.recoverAddress(hash, {
r: sig.r, _vs: sig._vs
});
assert.equal(addr, address, "Using r, _vs");
}

{
const addr = ethers.utils.recoverAddress(hash, {
r: sig.r, s: sig.s, recoveryParam: sig.recoveryParam
});
assert.equal(addr, address, "Using r, s and recoveryParam");
}
});
});
});
26 changes: 3 additions & 23 deletions packages/tests/src.ts/test-wallet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,30 +61,10 @@ describe('Test JSON Wallets', function() {
});

describe('Test Transaction Signing and Parsing', function() {
type TestCase = {
name: string;
accountAddress: string;
privateKey: string;

signedTransaction: string
unsignedTransaction: string;

signedTransactionChainId5: string
unsignedTransactionChainId5: string;

nonce: number;
gasLimit: string;
gasPrice: string;
to: string;
value: string;
data: string;
};
type TestCaseKey = 'nonce' | 'gasLimit' | 'gasPrice' | 'to' | 'value' | 'data';

function checkTransaction(parsedTransaction: any, test: TestCase): any {
function checkTransaction(parsedTransaction: any, test: TestCase.SignedTransaction): any {
let transaction: any = { };

['nonce', 'gasLimit', 'gasPrice', 'to', 'value', 'data'].forEach((key: TestCaseKey) => {
['nonce', 'gasLimit', 'gasPrice', 'to', 'value', 'data'].forEach((key: (keyof TestCase.SignedTransaction)) => {
let expected = test[key];

let value = parsedTransaction[key];
Expand Down Expand Up @@ -124,7 +104,7 @@ describe('Test Transaction Signing and Parsing', function() {
}


let tests: Array<TestCase> = loadTests('transactions');
let tests: Array<TestCase.SignedTransaction> = loadTests('transactions');
tests.forEach((test) => {
it(('parses and signs transaction - ' + test.name), function() {
this.timeout(120000);
Expand Down

0 comments on commit 4151c0e

Please sign in to comment.