Samourai Wallet Address Reuse Bug

On the 12th of June this year, I was approached by a Samourai Wallet user who had noticed multiple instances of address reuse in his wallet. According to this user, the behavior occurred automatically multiple times without any manual intervention on their part. These instances displayed a pattern of the same address being reused every time reuse occurred. Upon asking the user a few more questions, it was confirmed that the address in his wallet being reused was specifically the 0 index derivation path for that address type. This was peculiar, and I suspected that the core issue related to how the client handled indexing of most recently used addresses.

After my investigation I contacted a Samourai developer about the issue, and we agreed to a disclosure date of July 15th — exactly one month later. While they disclosed some aspects of the issue themselves yesterday, I still want to provide my side of the picture. Here is word-for-word excerpts of the disclosure I made to the Samourai developer, with additional explanation.

I’m pretty sure I have an idea of whats causing it, and this is an issue for both users of your backend, or Dojo instances, as the issue could be caused both by network connection errors or bad data from the backend.

https://code.samourai.io/wallet/samourai-wallet-android/-/blob/develop/app/src/main/java/com/samourai/wallet/send/SendFactory.java#L694 This function here is grabbing an index for change addresses, but it depends on the index value in the address factory. I think that is fed from the API Factory, for example here: https://code.samourai.io/wallet/samourai-wallet-android/-/blob/develop/app/src/main/java/com/samourai/wallet/api/APIFactory.java#L2607

It seems that setHighestPostChangeIdx is only called from API Factory if the HTTP request succeeds and the response body is valid JSON, if not a stack trace is printed to the debug log, and the JSON is nulled. In which case this doesn’t run: https://code.samourai.io/wallet/samourai-wallet-android/-/blob/develop/app/src/main/java/com/samourai/wallet/api/APIFactory.java#L259

This is a very serious issue at scale, so I’m reaching out to inform you in private first so that the issue can be replicated and fixed. I think that by enforcing a requirement that the index value can only increase, and persisting it in storage, should help address the issue.

At a high level, every HD wallet requires an index to keep track of “where the wallet is” in the derivation path for that address type, i.e. which address was used last, and which address to use next. In the instance of Samourai wallet, this is provided in a JSON object fed by the back end. However, I discovered a lack of error handling, where the JSON object can be nulled (leading to no index being set, causing the wallet to reuse addresses starting from index 0) in any situation where the client receives malformed data or any network connection disruption occurs. This could be solved with local safety checks and proper error handling, but none were in place.

Within the last one-month period, I have made three requests for updates on whether the Samourai team had been able to replicate the issue. Their replies were short, and did not give me any clarity regarding the progress of their investigation.​​​​​​​

Yesterday, they published their own disclosure, but it made no mention or refutation of the particular issue that I had explicitly pointed out to them. This is troubling; as you may recall, the lack of error handling mechanisms for data requests from external sources is similar to the architectural flaw with Blockchain.info’s (now Blockchain.com) reliance on random.org., causing entropy failure which resulted in users’ private keys being predictable by anyone.​​​​​​​ [https://www.reddit.com/r/Bitcoin/comments/aom95b/a_fun_little_memory_about_blockchaininfo/ | https://www.reddit.com/r/Bitcoin/comments/ch7nun/samouraileaks_part_3_is_randomorg_random_enough/].

This problem can be caused by connectivity issues or invalid responses due to an error in the back end, which leaves the client vulnerable not just to a malicious back end but also a network intermediary. (To be clear, I am not alleging that the Samourai developers would engage in such an attack. I am just pointing out that it is possible to intentionally trigger this behavior in such a manner. This is equally true of someone else’s Dojo instance.)

Instead of addressing the issue directly, their patch is to perform better filtering of transactions on the back end.​​​​​​​ Thankfully, this time, it was not something sensitive enough to put funds at risk. But given the nature of Samourai Wallet as a privacy tool, this is still disconcerting. Ultimately, every time the wallet is started, it starts with the default index of 0. Unless the client manages to successfully query the index from the back end, this default value will be used for constructing new transactions (this value is not persisted on the device). No errors are displayed to the user in case of query failure. The client will also blindly accept whatever it is fed by the back end without question.​​​​​ Instances of this address reuse are observable on the blockchain beyond what the user who reported this to me experienced.

From what I see, Samourai has decided to patch the back end with new filtering criteria that checks transactions for address reuse on Dojo or Samourai’s servers before broadcasting. This could potentially leave out fringe cases, such as users who create transactions and broadcast through other means. (It’s also important to mention that the Whirlpool Client does not suffer from this issue, as it persists the relevant index values locally.)

The last thing I want to say is a general point on how people engage with each other in this space. Over the past year or two, the Samourai team has devoted large amounts of time and resources towards analyzing flows of coins through their competitor, Wasabi. They blame them for post-mix behavior that is consciously taken by end users (which is impossible to prevent in a non-custodial wallet). Meanwhile, their own tools were exhibiting instances of address reuse due to poor architecting and error handling mechanisms. To say “throwing stones from a glass house” is, in my opinion, an understatement.

Due to the way they combatively engage with many other projects in the space, I suspect that there are fewer eyes checking their code base for issues than there otherwise would be. Ultimately it’s the users and not the competitor that suffers most because of the dynamics this environment creates.

This is a small subset of the overall transctions I sent Samourai developers (200 TXes) some off which were spot checked manually.

This is a list of the 5113 reused scripts in post-mix spends I was able to identify.

I will be investigating the differences between the number of reused addresses in my and Samourai’s analysis and looking for potential false positives, and follow up when finished.

ADDENDUM(07–16–2020): Samourai’s analysis of false positives in the overall set of address reuse is 100% correct. After the initial communication from the affected user who contacted me, I reached out to Nothingmuch to see if he could assist in a deeper analysis without me having to share the TXIDs relating to the affected user and to see if the issue existed beyond the scope of this single user. Nopara’s Dumpling data set was used to analyze all Samourai post-mix spends for instances of address reuse because it was readily available (as a side note it is important to consider only post-mix transactions were analyzed for reuse, the original affected user brought to my attention a post-mix transaction, so that is the transaction set that was analyzed). The reason for the disparity in reuse count was a failure to properly filter instances of “reuse” that equated to a script occuring once as an output, and being spent once as an input. Samourai’s count of 441 addresses reused is correct, not the number 5113 I arrived at.

This however does not change the nature of the bug in the client, or the severity of it for individual affected users. I still strongly believe an appropriate patch would be to persist the relevant wallet indexes locally in the same manner the Whirlpool desktop client does. I have put all of this out into the public transparently, with all the necessary data to verify both the reuse on chain and the code relating to the bug, and have described above the cause of the ascertained error in my numbers. I have no intent of engaging in slap fights or arguments on social media, everything necessary to verify this issue is right here, and what any individual chooses to do with that is their decision to make.