search

gnosis / safe-browser-extension

MIT License
18 stars 8 forks source link

Browser extension transaction confirmations fails with a recovered address #133

Closed MicaelaPesado closed 5 years ago

MicaelaPesado commented 5 years ago

Prerequisites Having an existent address created in another version or in the same one.

Description When trying to made a transaction with a recovered address connected with the extension, the app send the notification to the browser, we can confirm it, but after the confirmation, the app keeps asking for it.

Environment

Steps to reproduce

  1. Recover a safe with the extension connected.
  2. Scync the app with the extension.
  3. Make a sending transaction.
  4. Confirm the notification in the browser extension.
  5. Observe. Actual behaviour When sending the transaction, the notification is correctly sended from the app to the Browser extension. The user is able to confirm or delate the transaction, but the confirmation always seams to be pending on the app. Also when re-sending the confirmation request and confirming it with the extension the app keeps awaiting.

Additional context: Mobile device: Motog 6 Play and Iphone X Safe Android version: 1.5.0 Safe iOS version: 1.3.1 (5)

sche commented 5 years ago

@posthnikova has a bug on production safe 2019-06-24_17-20-37

  1. The transaction is initiated in the app
  2. Then chrome extension receives a push
  3. But when confirming with the extension 404 error happens

This is a payload from the extension back to the app

{“devices”:[“0x91B8C4Da89f97605629E667adbf4708b3e49cb29",“0x6350dfb59f1989f73217E6bCe52140545959b59D”,“0x4C26B7449673D25C10e28c6434C823a4C05cF460",“0x9df121EFd57CB137887fcc7DcE1A9494f855BBcC”],“message”:“{\“type\“:\“confirmTransaction\“,\“hash\“:\“0xc89eab5c080cdf739924b0449d144d4146681dde50ad96fe706cc01f1de1aec5\“,\“r\“:\“74761268592192215676756433001840596381198580505139451716256614252486979330619\“,\“s\“:\“56787045967158319603808067137115287620183062671248866148282704669315335300743\“,\“v\“:\“28\“}”,“signature”:{“r”:“34457886007192563953528535876175844814954775392570717264534771854369390523592",“s”:“20579455339284296105930722817793147918604786683026504116188587813862176935781",“v”:27}}
Should be the sender of a failed call be among devices?
sender: 0xd08f600d8318930cdc34C075e3DC6E59F0f250F9, devices: [‘0x91B8C4Da89f97605629E667adbf4708b3e49cb29’, ‘0x6350dfb59f1989f73217E6bCe52140545959b59D’, ‘0x4C26B7449673D25C10e28c6434C823a4C05cF460’, ‘0x9df121EFd57CB137887fcc7DcE1A9494f855BBcC’]
rmeissner commented 5 years ago

That looks like the extension uses a wrong account, since I would assume the sender should be 0x635...

less than 4 seconds to press the confirm button .... not bad :D

rmeissner commented 5 years ago

@MicaelaPesado did you use a newly installed chrome extension?

sche commented 5 years ago

That is Sasha's error. She has some transaction already sent with this extension. And after updating to iOS 1.3.0 this was the first time she tried to execute a transaction.

rmeissner commented 5 years ago

@sche I know that this was sashas error, still want to know if @MicaelaPesado used a clean extension ;)

rmeissner commented 5 years ago

This seems to be a bug related to the "new owner per safe" feature. The index stored is currentIndex + 1 when the push arrives, but this is error prone (what if a different safe was created in between, what if you resync a deleted safe) @germartinez this is something we need to fix. When the push arrives we need to check which index matches the owner of the safe and use that index. I would suggest just iterate decreasing from the currentAccountIndex until we find one (or error if we don't find one)

MicaelaPesado commented 5 years ago

@sche I know that this was sashas error, still want to know if @MicaelaPesado used a clean extension ;)

I had the extension already installed in the browser, also with some other addresses connected; but the safe we use to do this transactions, have been connected just before the transactions. Do you want us to test it with a clean extension?

rmeissner commented 5 years ago

I guess a clean transaction might not have the issue, but would be interesting to know :)

MicaelaPesado commented 5 years ago

I guess a clean transaction might not have the issue, but would be interesting to know :)

I tested whit the address we have already installed on the android device, and whit the cleaned extension it works fine. Then I test removing the addresses from the device, recovering a safe, connecting the browser extension, and making a transaction, and it works without any problem, too. Also, the address I recovered secondly, was connected to another browser extension, different from what I am using to test now.

DmitryBespalov commented 5 years ago

I can reproduce the issue by doing the following:

  1. Create, or restore, or connect browser extension to the safe
  2. In the same browser extension, select "Connect to a new safe"
  3. In the app, select "replace 2-factor authentication" from menu
  4. Connect with the qr-code displayed in the extension

  5. Execute the replace 2fa transaction.

    Expected: the newly generated safe owner is used Actual: old owner is used (the same safe notificaiton did not replace with the new owner).

The same behavior happens if you recover the same safe twice but do not remove the safe from the extension and use "connect new safe" instead.