Audited Electrum - Found a potential issue

[profitgenerator212]

I am no cryptographic expert, and I am moderately good in python, but I still audited the Electrum Source code to the best of my knowledge:

https://steemit.com/programming/@profitgenerator/electrum-bitcoin-wallet-code-audit


And I have found some issues:

It's the custom_entropy parameter of the make_seed function.

I kind of understood what this code does, it basically replaces an X amount of entropy from the PRNG number with the number you specify there, where X is the entropy difference between the num_bits and the entropy value of that integer.

Now there is 1 issue here:

Code:

n_custom = int(math.ceil(math.log(custom_entropy, 2)))

The math.log(custom_entropy, 2) is not the entropy of the custom number. Entropy is the "size of the haystack" basically.

Entropy is the Shannon Entropy, so if we pick 1 number out of 2⁵⁰, that is 50 bits of entropy, but just taking the log₂ value of a number is not it's entropy, it's more like it's bit size, it's bit length in binary, but not it's entropy.

This would assume that the binary length of the custom_entropy string is the same as the entropy value of it. Which is a wild assumption, not really true in most cases. I mean what is the entropy of a string 1111111111111111111111111111111111111111111111111111111111111111111, probably zero yet it's length is 6 bits. So this is a stupid assuption to make.

I think this custom entropy function should be designed or removed entirely, it places false trust in the entropy of what people supply to it. Or maybe it should be added not subtracted:

Code:

n = max(16, num_bits + n_custom)

The second issue is the multiplication part:

Code:

custom_entropy * (my_entropy + nonce)

I am no cryptographic expert, but I have researched this subject, it turns out that multiplication lowers entropy.

So multiplying 2 numbers gives massively less entropy than concatenating them. In fact the only way to preserve or add entropy is to concatenate strings or encode them into different ways.

So in this case I think this part of the code is flawed. Either some cryptographic expert vets this code or I think it should be removed since it's kind of looks like flawed to me.

[ThomasV]

The first issue is not with the code, but with the user's assumptions.
That is the very reason why custom entropy is not proposed in the GUI.

The second issue with custom entropy and the multiplication is real; thank you for pointing it out.

I too believe that the custom entropy option should be removed from the code.
Actually, I did remove it 6 months ago:
https://github.com/spesmilo/electrum/commit/e0c38b31b40b42138527e9fd3f4bad78e0b12802

and I later reverted that commit because users were complaining.

[profitgenerator212]

The first issue is not with the code, but with the user's assumptions.
That is the very reason why custom entropy is not proposed in the GUI.

The second issue with custom entropy and the multiplication is real; thank you for pointing it out.

I too believe that the custom entropy option should be removed from the code.
Actually, I did remove it 6 months ago:
https://github.com/spesmilo/electrum/commit/e0c38b31b40b42138527e9fd3f4bad78e0b12802

and I later reverted that commit because users were complaining.

Yes, but since most people will just type there their birthdate, their spouses birthdate, their kids birthdate, their marriage date, their phone number ....whatever, that could be a long concatenated string of many numbers, that might look like strong, but it actually has 0 entropy since all of the dates are public if associated with the user.

Look I am not crypto expert but I have researched some basic cryptographic concepts as a part of my due diligence when investing in cryptocurrencies, and I have analyzed the electrum wallet to the best of my knowledge, and this code struck me as odd.

So I am glad that I helped you fix this issue.