Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safe property access / allow transaction with undefined to fields, in signTransaction #98

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Safe property access / allow transaction with undefined to fields, in signTransaction #98

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c148ba
Safe property access / allow transaction with undefined to fields, in…
danjm Aug 27, 2021
5a754be
Revert "Safe property access / allow transaction with undefined to fi…
danjm Aug 27, 2021
8f2195c
Remove passage of to param to ledger bridge, which will not be needed…
danjm Aug 27, 2021
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
3 changes: 1 addition & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,7 @@ class LedgerBridgeKeyring extends EventEmitter {
rawTxHex = tx.type === 0 || !tx.type
? ethUtil.rlp.encode(tx.getMessageToSign(false)).toString('hex')
: tx.getMessageToSign(false).toString('hex')

return this._signTransaction(address, rawTxHex, tx.to.buf, (payload) => {
return this._signTransaction(address, rawTxHex, tx.to && tx.to.buf, (payload) => {
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like passing in nothing for toAddress will result in the to field passed to the Ledger device being set to 0x0. That doesn't seem right 🤔 . Should _signTransaction omit the to field instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

Actually, after merging #95, the to field isn't even needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have simply removed the to parameter in 8f2195c

Copy link
Member

Choose a reason for hiding this comment

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

That works! Though if you wanted to remove the dependency on #95, I think this would also be safe to merge if to was omitted for contract deployments, but passed in otherwise.

// Because tx will be immutable, first get a plain javascript object that
// represents the transaction. Using txData here as it aligns with the
// nomenclature of ethereumjs/tx.
Expand Down
35 changes: 35 additions & 0 deletions test/test-eth-ledger-bridge-keyring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ const newFakeTx = TransactionFactory.fromTxData({
data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
}, { common, freeze: false })

const newFakeTxWithoutTo = TransactionFactory.fromTxData({
nonce: '0x00',
gasPrice: '0x09184e72a000',
gasLimit: '0x2710',
value: '0x00',
data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
}, { common, freeze: false })

chai.use(spies)

describe('LedgerBridgeKeyring', function () {
Expand Down Expand Up @@ -483,6 +491,33 @@ describe('LedgerBridgeKeyring', function () {
expect(returnedTx).to.have.property('r')
expect(returnedTx).to.have.property('s')
})

it('should support transactions with undefined to fields', async function () {
await basicSetupToUnlockOneAccount()
// Signature will be invalid due to not having private key / public key
// pair for testing.
sandbox.on(newFakeTxWithoutTo, 'verifySignature', () => true)
sandbox.on(TransactionFactory, 'fromTxData', () => {
// without having a private key/public key pair in this test, we have
// mock out this method and return the original tx because we can't
// replicate r and s values without the private key.
return newFakeTxWithoutTo
})
sandbox.on(keyring, '_sendMessage', (msg, cb) => {
assert.deepStrictEqual(msg.params, {
hdPath: "m/44'/60'/0'/0",
to: ethUtil.bufferToHex(undefined),
tx: ethUtil.rlp.encode(newFakeTxWithoutTo.getMessageToSign(false)).toString('hex'),
})
cb({ success: true, payload: { v: '0x25', r: '0x0', s: '0x0' } })
})

const returnedTx = await keyring.signTransaction(fakeAccounts[0], newFakeTxWithoutTo, common)
expect(keyring._sendMessage).to.have.been.called()
expect(returnedTx).to.have.property('v')
expect(returnedTx).to.have.property('r')
expect(returnedTx).to.have.property('s')
})
})
})

Expand Down