The logic in bytes/splitSignature is unreliable

[ChALkeR]

Refs: c47d2eb#r55493545

splitSignature supports different input modes, like:

• string (canonical), with v
• string (legacy), with recid
• object, both v and recoveryParam present
• object, v present and recoveryParam isn't
• object, recoveryParam present and v isn't

The codepaths inside splitSignature() for those are inconsistent and don't check/normalize the input in a reliable way.

To check this, the easiest would be to try passing the output of one splitSignature call back into splitSignature in a different form and see what happens.

const ethers = require('ethers')
const assert = require('assert')

// string, v (canonical) : FAIL
{
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0xcafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e' // this is a vector from tests
    for (let v = 27; v <= 28; v++) {
        let signature = ethers.utils.concat([r, s, [v]]);
        let sig = ethers.utils.splitSignature(signature);
        assert.equal(sig.r, r, 'split r correctly');
        assert.equal(sig.s, s, 'split s correctly');
        assert.equal(sig.v, v, 'split v correctly');
        assert.equal(sig.recoveryParam, v - 27, 'split recoveryParam correctly');
        try {
          // The above passes, but this fails? Did we miss a check in string variant?
          ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v })
        } catch (e) {
          console.error('E1', e.message)
        }
    }
}

// string, recid (legacy) : FAIL
{   
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0xcafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e'
    for (let v = 27; v <= 28; v++) {
        let signature = ethers.utils.concat([r, s, [v - 27]]);
        let sig = ethers.utils.splitSignature(signature);
        assert.equal(sig.r, r, 'split r correctly');
        assert.equal(sig.s, s, 'split s correctly');
        assert.equal(sig.v, v, 'split v correctly');
        assert.equal(sig.recoveryParam, v - 27, 'split recoveryParam correctly');
        try {
          // The above passes, but this fails? Did we miss a check in string variant?
          ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v })
        } catch (e) {
          console.error('E2', e.message)
        }
    }
}

// object, v +, recoveryParam - : OK
{
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0x00fe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e'
    for (let v = 27; v <= 28; v++) {
        let sig = ethers.utils.splitSignature({ r, s, v });
        assert.equal(sig.r, r, 'split r correctly');
        assert.equal(sig.s, s, 'split s correctly');
        assert.equal(sig.v, v, 'split v correctly');
        assert.equal(sig.recoveryParam, v - 27, 'split recoveryParam correctly');
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v }) // passes
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, recoveryParam: sig.recoveryParam }) // passes
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v, recoveryParam: sig.recoveryParam }) // passes
    }
}

// object, v -, recoveryParam + : OK
{
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0x00fe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e'
    for (let v = 27; v <= 28; v++) {
        let sig = ethers.utils.splitSignature({ r, s, recoveryParam: v - 27 });
        assert.equal(sig.r, r, 'split r correctly');
        assert.equal(sig.s, s, 'split s correctly');
        assert.equal(sig.v, v, 'split v correctly');
        assert.equal(sig.recoveryParam, v - 27, 'split recoveryParam correctly');
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v }) // passes
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, recoveryParam: sig.recoveryParam }) // passes
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v, recoveryParam: sig.recoveryParam }) // passes
    }
}

// object, v = recid, recoveryParam - : FAIL
{
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0x00fe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e'
    for (let v = 27; v <= 28; v++) {
        // There is a branch for result.v === 0 || result.v === 1, which restores the recoveryParam
        // But the result of that operation is inconsistent with all other `splitSignature` output modes and can't be passed back!
        let sig = ethers.utils.splitSignature({ r, s, v: v - 27 });
        assert.equal(sig.r, r, 'split r correctly');
        assert.equal(sig.s, s, 'split s correctly');
        try {
          assert.equal(sig.v, v, 'split v correctly'); // THIS FAILS?
        } catch (e) {
          console.error('E3', e.message)
        }
        assert.equal(sig.recoveryParam, v - 27, 'split recoveryParam correctly');
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v }) // passes
        ethers.utils.splitSignature({ r: sig.r, s: sig.s, recoveryParam: sig.recoveryParam }) // passes
        try {
          // Can't be passed back
          ethers.utils.splitSignature({ r: sig.r, s: sig.s, v: sig.v, recoveryParam: sig.recoveryParam }) // FAILS
        } catch (e) {
          console.error('E4', e.message)
        }
    }
}

// object, v = recid, recoveryParam + : FAIL
{
    const r = '0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef'
    const s = '0x00fe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7ecafe1a7e'
    for (let v = 27; v <= 28; v++) {
        // commit c47d2eba4dc741eb5cb754c3ef5064b8ea7ac7cc added support for v === 0 || v === 1, but this
        // apparently doesn't work if recoveryParam is specified? Is that intentional?
        try {
          ethers.utils.splitSignature({ r, s, v: v - 27, recoveryParam: v - 27 });
        } catch (e) {
          console.error('E5', e.message)
        }
    }
}

Judging by the code, all of those modes were intended to be supported.

When the second call fails, I would expected the first call also to fail or to return the output in a normalized form.

[ricmoo]

Thanks. I’ll look into this. Any non-ambiguous input should be supported, and yes I want the output to normalize all values.

There are a lot of possible combinations, and I think I may have to visit simplifying the code. There also seems to be some weirdness in how some hardware wallet manufactures interpreted certain values if v.

In v6, this is a proper object with methods that can validate all the fields make sense.

[ChALkeR]

My main concern about that is that c47d2eb added support for v === 0 || v === 1 in object mode, but passing those in returns the object in an inconsistent state (i.e. which couldn't be passed back).

[ricmoo]

Fixing this now. The first two have an S that is out-of-range for canonical signatures, which is why it is failing. The first nibble must but less than 8.

The last few are a bug though, which I'm fixing now and will go out in the next version. :)

[ricmoo]

This should be fixed in 5.5.0. If you still have problems, let me know and please re-open this issue.

Thanks for your attention to detail! :)