signer/core: fix reference issue in key derivation #19827
Conversation
|
This seems like a bug in a specific derivation method. It should definitely not keep a pointer to the path, rather make a copy if it needs it. Where does it originate from though, Ledger and Trezor only use it for a request, but don't mutate it as far as I see it. |
|
Ah, indeed it does keep a reference. Please make a copy here: https://github.com/ethereum/go-ethereum/blob/master/accounts/usbwallet/wallet.go#L482. |
gballet
force-pushed
the
clef-sign-hw-key-derivation
branch
from
37c600b to
5f46edc
Compare
| @@ -479,7 +479,7 @@ func (w *wallet) Derive(path accounts.DerivationPath, pin bool) (accounts.Accoun | |||
|
|
|||
| if _, ok := w.paths[address]; !ok { | |||
| w.accounts = append(w.accounts, account) | |||
| w.paths[address] = path | |||
| copy(w.paths[address], path) | |||
There was a problem hiding this comment.
This doesn't make a new copy. It retrieves w.paths[address], which will be an empty slice (nil) and then copy into it, limited to the length of the smallest operand (i.e. nil, length of 0`) and return. I.e. this is a noop.
There was a problem hiding this comment.
yeah I just saw debugging the other PR 😞
There was a problem hiding this comment.
Alternatively, we can make a Copy method for accounts.DerivationPath, might come in handy long term for Clef too.
* signer/core: fix reference issue in key derivation * Review feedback
Currently in
cléclef, the derivation process reuses the same derivation path array as input to theDerivefunction. ThatDerivefunction creates an account and keeps a pointer to the derivation path for other purposes.Since the derivation path gets updated each time
Deriveis called, all accounts end up having the rug pulled from under their feet: the derivation path they point to will be that of the first non-valid account.So this PR simply makes sure that one derivation path is allocated each time.