[FIX] Rootstock address validation on transaction passed from external dapp

[ahsan-javaiid]

Description

Tally is rejecting Rootstock transactions passed from external dapps. For example, if an external dapp passes checksummed from and to fields in transaction payload then Tally process dies with below error log.

logger.ts:211 Error signing transaction Error: bad address checksum (argument="address", value="0xE6671D3846C57de1c559Ff2Da7bc0f4B4fEa9521", code=INVALID_ARGUMENT, version=address/5.5.0)
    at Logger.makeError (index.js:185:1)
    at Logger.throwError (index.js:194:1)
    at Logger.throwArgumentError (index.js:197:1)
    at getAddress (index.js:78:1)
    at Formatter.address (formatter.js:188:26)
    at SerialFallbackProvider.<anonymous> (base-provider.js:1559:1)
    at Generator.next (<anonymous>)
    at fulfilled (base-provider.js:5:43)

Reason

Because ether.js library rejects Rootstock addresses. Below is script to justify this:

import { ethers } from "ethers"
import { toChecksumAddress } from "@tallyho/hd-keyring"

const validRSKAddress = "0xE6671D3846C57de1c559Ff2Da7bc0f4B4fEa9521"

void (() => {
  try {
    const addr = ethers.utils.getAddress(validRSKAddress)
    console.log("Address validation passed",  addr)
  } catch (e) {
    console.log("ether rejected the address", e)
  }

// Solution: Validate using chainId is a correct way to validate Rootstock addresses
  console.log(
    "Is really valid rootstock address: ",
    isValidChecksumAddress(validRSKAddress, 30)
  ) // True
})()

function isValidChecksumAddress(address: string, chainId?: number): boolean {
  return toChecksumAddress(address, chainId) === address
}

Suggested solution

• Validate Rootstock transaction address fields using isValidChecksumAddress function at the point when dapp passes transaction to tally for signature
• After that make to and from fields to lowercase to skip the ether.js address validation Note: Above two points will only apply if network is Rootstock

Type of Change

• Fix

Checklist

• [x] Git hooks enabled
• [x] No lint errors
• [x] Git commit is signed

Latest build: extension-builds-2807 (as of Wed, 21 Dec 2022 11:24:53 GMT).

[ahsan-javaiid]

cc: @0xDaedalus @alepc253

[ahsan-javaiid]

Hi Tally team 👋 Just mentioning one other approach for this pull request.

Alternative approach:

1. Validate address fields using chainId approach for all the chains like in this PR but remove Roostock check
2. Lowercase the address fields in tx payload regardless of network (Remove Rootstock check in this PR) Eventually ehter.js address validation will be bypassed for all the chains and addresses will be validated as per chainId approach

Let me know whichever approach makes more sense to the team and feel free to share further thoughts.

[alepc253]

What is the best dapp to test this PR with?

Hi @jagodarybacka, we found this while porting an old dApp to make it work with Tally and Ledger but it was in dev/local environment (anyway, I'll try to find a productive example to share). In general, we take the approach of turning to lowercase in the dApp before sending the transaction but it would be great if this is also considered in Tally (because we can prevent errors and, in addition to this, remove a possible friction making Tally the easiest wallet to integrate your dApp on Rootstock).

[ahsan-javaiid]

cc: @0xDaedalus for review.

[ahsan-javaiid]

Hi Tally team:

Any thought's on considering this pull request. 🙏🏻

cc: @0xDaedalus

[Shadowfiend]

Will be working to get this merged early this week, thanks for the quick turnaround @ahsan-javaiid !

[alepc253]

@Shadowfiend this is the dapp where we found the issue: https://gnosis.rsk.co/ Video example attached: https://user-images.githubusercontent.com/3451282/234598148-6d28f8b6-b69d-464a-91d5-6952c801e1fb.mov