[Feature Request] Remove EIP-55 checks from atomic_bridge.move

[andygolay]

🚀 Feature Request

Instead of doing an EIP-55 checksum check, the Move modules in the framework should simply verify that Ethereum addresses are 20 bytes and nonzero, containing no spaces.

Motivation

• There was never a discussion of whether we should have EIP-55 checks onchain. No auditors suggested doing so when reviewing our older Move modules (which do not contain EIP-55 checks).
• Checksum validation is purely for user-facing convenience. It is better handled off-chain by wallets and clients.
• Validating checksums on-chain doesn't contribute to the security of the Ethereum address itself. It merely ensures adherence to a format that's already assumed correct when submitted.
• By requiring users to connect a wallet on the bridge front-end, and extracting the Ethereum address from that wallet, we can safely assume correctness an Ethereum address. As an additional check, the Move modules can verify that the Ethereum address is 20 bytes and nonzero.
• We were using this rev for aptos dependencies in the movement repository to test against. When opened as a Remove EIP 55 check from atomic_bridge_counterparty::lock_bridge_transfer_assets and use EventHandle for events  #87, discussion opposing it included the following:

"IMHO, this is a big mistake. Either you check compliance of addresses with EIP-55 everywhere or not.
What guarantees do you have that each address you get in transfer has been checked for EIP-55 compliance?
As I mentioned before, this is a very dangerous strategy to remove tests because something is supposed to have happened before.
It also adds a lot of assumptions to track (or forget about), and when we modify the code later on, there will be some issues.
I am strongly opposed to this kind of bypassing security devices."

from @franck44

and

I would agree making assumptions off chain isn't wise and I would also prefer to see validation on chain, in which we trust, where possible. If this is a PR to alleviate an operational requirement or to test then perhaps it should be either feature flagged or even deployed as a branch with this change and avoid merging to our trunk movement. The latter would be best practice I would say.

from @andyjsbell

So the concern is that validation is done on chain. Which is fine; we should check that the addresses are 20 bytes and nonzero, with no spaces. But EIP-55 checks don't make sense to do on-chain, because EIP-55 is meant to check for typographical errors made by humans.

See:

You can send ETH to any series of 20 bytes, so in that sense there are no invalid addresses.

All EIP 55 does is provide a checksum to be able to detect copying errors: changing a digit will invalidate the checksum. It doesn't say anything about whether there exists a private key for the address, whether anyone has a private key for that address or whether a smart contract is located at that address.

• Source: Stack Exchange

A user could enter the wrong Eth address and experience loss of funds regardless of whether EIP-55 checks happen in the Move modules. In my opinion, EIP-55 checks in the Move modules are not needed.

Pitch

• Instead of a full EIP-55 checksum check, simply check that the address is 20 bytes, nonzero and doesn't contain spaces. There's no need for a full-fledged EIP-55 check onchain.

Describe alternatives you've considered

We tried it with the EIP-55 checksum check. What happens is that between function calls, the relayer must convert lowercase addresses to checksummed addresses. This is hacky and could introduce errors avoided by simply passing along the lowercase address.

Are you willing to open a pull request? (See CONTRIBUTING)

Yes but the idea of removing EIP-55 checksums from the Move modules has been rejected so far. It should be discussed first before a PR is opened.

Additional context

• Issue 79 in the movement repository states, "we would like to have EIP checks across the relayer wherever we are handling ETH addresses, this is not so trivial because of the various From implementations in either direction." I don't see that as being necessary, because there is no manual copying of addresses by a human, on the relayer. It is sufficient to check that an address is 20 bytes, nonzero, and contains no spaces.

[franck44]

So I would argue that EIP-55 provides some assurances and not only to check human typos.
EIP-55 checks two things:

• the address has been computed using an algorithm. Someone ran an algorithm to compute the EIp55 address
• the address has not been tampered with. This can happen on-chain when you pass the address around as arguments to functions etc, so it is good to check its integrity.

And two more arguments:

1. I don't think Vitalik is the kind of person who would propose useless EIPs 😄
2. hardware wallets like Ledger Nano use EIP-55 compliant addresses (and the addresses are not input by the user, but in the hardware cold wallet).

Now if we decide to remove EIP-55 checks, which is perfectly reasonable and simple, we should probably check a few things:

• nonzero address (OK, if we send there, the funds are lost)
• length: 20 bytes/40 chars
• all chars (40 chars) should be hex digits.

[andygolay]

So I would argue that EIP-55 provides some assurances and not only to check human typos. EIP-55 checks two things:

• the address has been computed using an algorithm. Someone ran an algorithm to compute the EIp55 address
• the address has not been tampered with. This can happen on-chain when you pass the address around as arguments to functions etc, so it is good to check its integrity.

And two more arguments:

1. I don't think Vitalik is the kind of person who would propose useless EIPs 😄
2. hardware wallets like Ledger Nano use EIP-55 compliant addresses (and the addresses are not input by the user, but in the hardware cold wallet).

Now if we decide to remove EIP-55 checks, which is perfectly reasonable and simple, we should probably check a few things:

• nonzero address (OK, if we send there, the funds are lost)
• length: 20 bytes/40 chars
• all chars (40 chars) should be hex digits.

Yes, I think those three requirements would be reasonable middle ground. The requirements of nonzero and 40 hex digits seems fine. But that's an interesting point about hardware wallets using EIP-55. Ultimately if we decide to keep EIP-55, we could engineer solutions.

I mainly just want to the team to come to a decision about it so we have a reference point about it in an issue. Whatever the team decides, we can work with.

[andygolay]

@franck44 I've updated PR 87 to perform the checks based on the three requirements you mentioned: #87, checked that it passes Move unit tests, E2E tests, and Move Prover, and opened for re-review 🙏