search

LedgerHQ / wallet-connect-live-app

https://wallet-connect-live-app.vercel.app/
7 stars 6 forks source link

Wallet connect swap transactions fixed #78

Closed cgrellard-ledger closed 1 year ago

cgrellard-ledger commented 1 year ago

This PR fixes the issue that was causing some swap transactions to lose their data field resulting in users losing funds

Introduced on June 30 2023 by this commit (here is the JIRA ticket)

🗒️ Reasons for the pendingFlow

When we are connected to a dapp (paraswap for example) and we trigger an operation (either a sign message or a transaction) some dapp send the information of the operation to the wallet connect live app that then triggers the appropriate flow on Ledger Live. However some dapp also triggers a deeplink that opens Ledger Live and redirects to the live app which causes it to refresh and it reinitializes the transport layer of the wallet api client. When this occurs and a flow (either sign message or signAndBroadcast transaction) was already opened then the live app will never get the result of the flow once it is either rejected or finished successfully (this issue was initially reported by opensea) Screenshot 2023-07-20 at 14 07 59 On the following screen record we can see that once I trigger a swap from paraswap the flow opens up in ledger live directly but when the deeplink is then triggered (either manually when I click on Open Ledger Live or automatically if I had previously clicked on the checkbox above) then the live app is refreshed behind the sign transaction flow which resets the transport between Ledger Live and the live app Screen record : https://github.com/LedgerHQ/wallet-connect-live-app/assets/17146928/7f0d1a6b-f9ab-4237-a112-f91c7eb94a36 The pendingFlow stores the data of an ongoing operation (sign message or sign and broadcast transaction) and triggers the appropriate flow on Ledger Live with those data when the live app refreshes and a new transport layer has been initialized so that the live app can get the response of this flow once it's finished and send the result to the dapp which fixes the issue of this ticket

🐞 Explanation of the swap transaction critical bug

This bug occurred when triggering a pending flow to send a tx to Ledger Live when the live app was refreshed by a deeplink. We fetched the tx from the localStorage before sending it to ledger live but the data field being a Buffer its type was modified (from a Buffer to an Object) when being stored by zustand in the localStorage. So when we were getting it back at the start of the live app and sent it to Ledger Live this field was not a Buffer anymore but an object and when Ledger Live converted it to a string it resulted in the data being empty

Screenshot 2023-07-20 at 11 46 59

The consequence of that is the transaction being executed as a simple send to the dapp contract address instead of a swap (thus the users losing funds)

✅ Solution

Save the ethTx object (which has a type of EthTransaction and is only composed of strings) in the localStorage instead of the liveTx object (which has a type of EthereumTransaction and is composed of strings, BigNumbers and a Buffer). Then converting the ethTx to a liveTx just before sending the transaction to Ledger Live

Screenshot 2023-07-20 at 15 16 07 Screenshot 2023-07-20 at 15 19 13

That avoids any type modification and the data is still intact when converting the EthTransaction to a EthereumTransaction

🔃 How to reproduce the bug

{
    "id": "ledger-wallet-connect",
    "name": "Wallet Connect",
    "url": "https://wallet-connect-live-app-git-support-wc-without-00718d-ledgerhq.vercel.app/",
    "params": {
        "networks": [
            {
                "currency": "ethereum",
                "chainId": 1
            },
            {
                "currency": "bsc",
                "chainId": 56
            },
            {
                "currency": "polygon",
                "chainId": 137
            }
        ]
    },
    "homepageUrl": "https://walletconnect.com/",
    "icon": "https://forum.zeroqode.com/uploads/default/original/2X/e/e363c6521db27335d44c1134d230b8992792dde4.png",
    "platform": "all",
    "apiVersion": "^2.0.0",
    "manifestVersion": "1",
    "branch": "stable",
    "categories": ["bridge", "defi"],
    "currencies": ["ethereum", "polygon", "bsc"],
    "content": {
        "shortDescription": {
            "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
        },
        "description": {
            "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
        }
    },
    "permissions": [
        "account.list",
        "account.request",
        "message.sign",
        "transaction.sign",
        "transaction.signAndBroadcast",
        "wallet.userId",
        "wallet.info"
    ],
    "domains": ["http://*", "https://*"]
}

Screenshot 2023-07-20 at 16 06 27 Screenshot 2023-07-20 at 16 06 37

Screen record : https://github.com/LedgerHQ/wallet-connect-live-app/assets/17146928/a9588dd7-964d-4e7c-8b59-14d62beb6df5

🧪 Testing the fix

{
    "id": "ledger-wallet-connect",
    "name": "Wallet Connect",
    "url": "https://wallet-connect-live-app-git-bugfix-swap-transactions-ledgerhq.vercel.app/",
    "params": {
        "networks": [
            {
                "currency": "ethereum",
                "chainId": 1
            },
            {
                "currency": "bsc",
                "chainId": 56
            },
            {
                "currency": "polygon",
                "chainId": 137
            }
        ]
    },
    "homepageUrl": "https://walletconnect.com/",
    "icon": "https://forum.zeroqode.com/uploads/default/original/2X/e/e363c6521db27335d44c1134d230b8992792dde4.png",
    "platform": "all",
    "apiVersion": "^2.0.0",
    "manifestVersion": "1",
    "branch": "stable",
    "categories": ["bridge", "defi"],
    "currencies": ["ethereum", "polygon", "bsc"],
    "content": {
        "shortDescription": {
            "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
        },
        "description": {
            "en": "WalletConnect is an open source protocol for connecting decentralised applications to mobile wallets with QR code scanning or deep linking. V2 introduces new features, including the ability to connect to multiple dapps in parallel with multiple accounts. It's important to note that not all dapps currently support V2"
        }
    },
    "permissions": [
        "account.list",
        "account.request",
        "message.sign",
        "transaction.sign",
        "transaction.signAndBroadcast",
        "wallet.userId",
        "wallet.info"
    ],
    "domains": ["http://*", "https://*"]
}

Screen record : https://github.com/LedgerHQ/wallet-connect-live-app/assets/17146928/45469a42-7ea1-4231-ab93-6521376928af

🔬 How could this have been avoided

The pendingFlow has been carefully tested manually by both the devs and the QA but this issue only happened when executing specific steps in the right order and was only visible for transactions carrying some data (such as swaps) and these were not part of the manual testing plan of the pendingFlow. However it would have been caught before reaching production if we had developed proper automated tests for the pendingFlow during the development phase and it could definitely have been avoided.

🕵️ How can we improve the investigation

More detailed logs on both the Live App and Ledger Live would have helped us identify which part was at the cause of the issue and it would have reduced the number of teams and people involved in the investigation. Also some tests on all the potential sources of the issue (live app, wallet api, ledger live, ...) where we could have sent the exact data of the transaction that had the issue could have eliminated some of them and saved us some resources as well as direct our focus to the remaining potential sources.

🔮 What can we do to avoid that in the future

Now that the fix has been implemented and released, it shouldn't be possible anymore for the data field of a transaction to be erased by the pendingFlow. We also added a double security check to throw an error and not execute the signAndBroadcast method if the data field were to be erased at some point in the live app. In addition, the automated test covering this particular issue is currently in development and we are actively working on defining the whole testing strategy to cover the wallet connect live app and putting in place all the tools that are needed to properly test it. The automated tests will now be part of the definition of done of the upcoming features and bugfixes that will be implemented.

mcayuelas-ledger commented 1 year ago

We will need to merge Sentry PR with this one to being able to monitor the fix.

Currently Sentry doesn't work properly. Due to Authentication, I'm not able to track in "production" mode just locally @KVNLS