Remove EIP 55 check from atomic_bridge_counterparty::lock_bridge_transfer_assets and use EventHandle for events

[andygolay]

Description

• Because initiateBridgeTransfer will already have been successfully called on the Eth initiator side, there is no need to check EIP-55 compliance in atomic_bridge_counterparty::lock_bridge_transfer_assets.
• The bridge team has decided for event listening it would make sense to use event handles rather than module events for this iteration of the bridge.
• This PR adds an optional function ethereum_address_no_eip55 containing a check assert_40_char_hex to check that the address is a nonzero 40-character hex string.

How Has This Been Tested?

• Move unit tests: aptos move test passes for all tests in aptos-move/framework/aptos-framework/sources
• E2E Move tests: Navigate to aptos-move/e2e-move-tests and run cargo test bridge.
• Move prover (aptos move prove) results in success.

Key Areas to Review

Note: I don't think this is an ideal long-term solution. If it's helpful for launching the bridge with consistent event monitoring in the short-term, for the sake of consistency with how event monitoring was done with the first iteration of Move modules.

Type of Change

• New feature
• Bug fix
• Breaking change
• Performance improvement
• [ x] Refactoring
• Dependency update
• Documentation update
• Tests

Which Components or Systems Does This Change Impact?

• Validator Node
• Full Node (API, Indexer, etc.)
• Move/Aptos Virtual Machine
• [x ] Aptos Framework
• Aptos CLI/SDK
• Developer Infrastructure
• Move Compiler
• Other (specify)

Checklist

• [ x] I have read and followed the CONTRIBUTING doc
• [ x] I have performed a self-review of my own code
• [ x] I have commented my code, particularly in hard-to-understand areas
• [ x] I identified and added all stakeholders and component owners affected by this change as reviewers
• [ x] I tested both happy and unhappy path of the functionality
• [x ] I have made corresponding changes to the documentation

[franck44]

Because initiateBridgeTransfer will already have been successfully called on the Eth initiator side, there is no need to check EIP-55 compliance in atomic_bridge_counterparty::lock_bridge_transfer_assets.

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.

[0xmovses]

@franck44 see the issue :
movementlabsxyz/movement#729

We are working toward deploying the new version of the bridge relayer on Thursday, we can follow up with EIP-55 implementations anytime between now and next week.

[andygolay]

Because initiateBridgeTransfer will already have been successfully called on the Eth initiator side, there is no need to check EIP-55 compliance in atomic_bridge_counterparty::lock_bridge_transfer_assets.

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.

Weren't the concerns you previously mentioned focused on the Movement -> Eth direction?

Why do you think EIP-55 compliance checks should be everywhere or not? Shouldn't they only be where there's a possibility that a user manually enters an Eth address they didn't intend to use?

[0xmovses]

In our case the address will always come from the wallet in the browser, no address would ever be entered manually. It only protects against the case that somewhere the address could be manipulated by some code, our own, or malicious.

[0xmovses]

@andygolay Just wanted to confirm, when you set the e2e relayer tests to use this version of the framework, the e2e tests on the relayer for the transfer Eth -> Movement are passing, right?

[andygolay]

In our case the address will always come from the wallet in the browser, no address would ever be entered manually. It only protects against the case that somewhere the cases would be manipulated by some code, our own, or malicious.

It wouldn't even protect against that, as far as I can tell. If an address is manipulated by some code between initiateBridgeTransfer on the Eth initiator side and lock_bridge_transfer_assets on the Movement counterparty side, then we've got bigger problems than a typo check. BridgeTransferInitiatedEvent is emitted atomically so it should have no reason for data corruption, and our Rust code is designed to pass on the details in a way that should not introduce typos.

One thing to consider @franck44 is that in the emitted event, it's taking the value of the intiator Eth address from msg.sender which will always be lowercase (see https://ethereum.stackexchange.com/questions/52253/contract-creation-not-storing-address-as-mixed-case-using-web3-1-0)... so the only way to get the address into a checksummed form would be the hack you were opposed to (where we convert a lowercase address to a checksummed address before calling atomic_bridge_counterparty::lock_bridge_transfer_assets).

If someone calls lock_bridge_transfer_assets on the Movement side without having called initiateBridgeTransfer on the Eth side, then they must be the bridge operator, first of all, because only the operator can call lock... and we aren't manually entering Eth addresses. If it were the case that a bridge operator were so incompetent that they manually entered an Eth address for some reason, then that would be a much bigger issue.

If lock_bridge_transfer_assets isn't called on the Move side after initiateBridgeTransfer, then the bridge operator should never manually intervene. Rather, they should allow the transfer to expire, so the initiator can be refunded. Because initiateBridgeTransfer was called successfully, there is a 0% chance that the Eth address was invalid. If someone chose a different account in their Eth wallet than they intended to use, then there would be no way for EIP-55 to protect against that.

One point @0xPrimata raised is that in the Eth -> Movement direction, checking the Movement address a user enters is something worth considering. But I don't see the need to check the Eth address in the Move contract after a successful initiateBridgeTransfer call especially if we are using a connected Eth wallet on the frontend.

[andygolay]

@andygolay Just wanted to confirm, when you set the e2e relayer tests to use this version of the framework, the e2e tests on the relayer for the transfer Eth -> Movement are passing, right?

No, sent you a message in Slack about those tests.

[0xmovses]

Ok I think before this goes in we want to make sure the e2e on relayer is passing, as we may need to modify further here.

[andygolay]

Ok I think before this goes in we want to make sure the e2e on relayer is passing, as we may need to modify further here.

Yeah agreed 100%

[andygolay]

Set to draft until relayer tests pass

[andyjsbell]

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.

[andygolay]

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.

Regarding the branching, I'll defer to project management. That seems good, to keep it out of movement as it uses event handles which are depreciated.

As for validation, it's not an assumption about anything off-chain, that the bridge operator is the only one who can call atomic_bridge_counterparty::lock_bridge_transfer_assets. Is there some risk of typographical errors that you think is mitigated by an EIP-55 checksum check in that function call? The bridge operator should not be manually entering any Eth addresses. Regardless of whether someone called initiateBridgeTransfer on the Eth initiator side, if the bridge operator somehow calls atomic_bridge_counterparty::lock_bridge_transfer_assets with a wrong Eth address, isn't that a bigger issue than typos?

One note, that I think is more at the crux of the matter: EIP-55 does not validate an Ethereum address's existence or correctness (with respect to having a balance, transactions, or association any contracts). It is only a checksum to avoid typos when humans manually enter an address.

It makes sense in the Movement -> Eth direction because there a user, the initiator may manually enter an Ethereum address as the recipient.

But in the Eth -> Movement direction, and I'll pose this as a question to @franck44 as well:

Can you describe a scenario in which the initiator successfully calls initiateBridgeTransfer on the Eth initiator side, and then atomic_bridge_counterparty::lock_bridge_transfer_assets is called with an incorrect Ethereum address as the initiator, due to typographical errors, leading to the initiator losing money?

[0xmovses]

Keeping this open just for visibility as we are using this revision elsewhere

[l-monninger]

@0xmovses @franck44 @andygolay Otherwise this is defacto "shipping what we have."