[Bridge] [Solidity] Remove BridgeTransfer Struct

[Primata]

Is your feature request related to a problem? Please describe.
Our bridge is currently extremely expensive as shown in our gas analysis: #842

We also need to assert that the bridge values are being passed correctly and keccak256 of all params can handle it. keccak256 is a cheap process, especially for Ethereum Mainnet, keeping all of it's logic in memory.
#830

Describe the solution you'd like
One solution is to drop the struct completely and only store a mapping of the bridgeTransferId to it's state:

mapping(bytes32 bridgeTransferId => uint8 state) bridgeTransfers;

then, on the initiator contract initiateBridgeTransfer(recipient, amount, hashLock) the bridgeTransferId is created using params and stored:

uint256 initialTimestamp = block.timestamp;
bytes32 bridgeTransferId = keccak256(abi.encodePacked(msg.sender, recipient, amount, initialTimestamp, hashLock, ++nonce));
bridgeTransfers[bridgeTransferId] = 1;

emit BridgeTransferInitiated(initiator, recipient, amount, initialTimestamp, hashLock, nonce);

subsequent calls will use emitted values by BridgeTransferInitiated();

on the counterparty contract lockBridgeTransfer(bridgeTransferId, recipient, amount, initialTimestamp, hashLock, nonce)

bytes32 bridgeTransferIdToVerify = keccak256(abi.encodePacked(initiator, recipient, amount, initialTimestamp, hashLock, nonce));
require(bridgeTransferId == bridgeTransferIdToVerify, InvalidHash());
// then store the bridgeTransferId
bridgeTransfers[bridgeTransferId] = 1;

user calls lockBridgeTransfer(bridgeTransferId, recipient, amount, initialTimestamp, hashLock, nonce, preImage)

bytes32 bridgeTransferIdToVerify = keccak256(abi.encodePacked(initiator, recipient, amount, initialTimestamp, hashLock, nonce));
require(bridgeTransferId == bridgeTransferIdToVerify, InvalidHash());
// change the bridgeTransferId state to completed on counterparty
bridgeTransfers[bridgeTransferId] = 2;

Then we are finally able to call on initiator completeBridgeTransfer(bridgeTransferId, recipient, amount, initialTimestamp, hashLock, nonce, preImage)

bytes32 bridgeTransferIdToVerify = keccak256(abi.encodePacked(initiator, recipient, amount, initialTimestamp, hashLock, nonce));
require(bridgeTransferId == bridgeTransferIdToVerify, InvalidHash());
require(keccak256(preImage) == hashLock, InvalidHash());
// change the bridgeTransferId state to completed on initiator
bridgeTransfers[bridgeTransferId] = 2;

Describe alternatives you've considered
Completely rebuilding the bridge or keeping it as it is.

Additional context

[franck44]

@0xPrimata

Our bridge is currently extremely expensive as shown in our gas analysis: issue 842

Where is the analysis?
issue 842 proposes to do an analysis with forge but this has not been done yet.

Maybe we should profile the contracts first and then we identify the culprit section as @0xmovses suggested.

[Primata]

Responded to issue 842 with gas report

[franck44]

@0xPrimata Thanks for that.

The issue with not storing the bridge transfer details (in both direction) is that we don't have the data (details) for the refund.

How do you retrieve the details of a transfer in the refund function?

[Primata]

@franck44 same way, confirm by hashing with the original data. That will create a keccak256 that confirms that the user is inputting legitimate arguments. Then, bridgeTransfers[bridgeTransferId] will be at state 1 and you check that timelock has passed.

[franck44]

@0xPrimata Read the question again please:

The issue with not storing the bridge transfer details (in both direction) is that we don't have the data (details) for the refund.
How do you retrieve the details of a transfer in the refund function?

1. If we have a permissioned refund, we may not know the details of the transfer, and we need to get them from somewhere when we call refund.
2. if the user can call refund, we use the nonce in the hash and we need to communicate this nonce to the user, and they need it on top of their transfer details to re-compute the hash. That's a complicated set up.
3. the original intention was to save gas because 400K gas was too expensive, but the reality is that it costs 130K to 200K gas, so there is probably no need to optimise anymore.

[Primata]

Sorry, so we have an API that stores events, we would store the initaite bridge event and that data would serve to call refund.
We can also have it in local memory while the functions are called.
Or we could store it in local storage.
Or we could retrieve it from etherscan's API by taking the function call arguments.
Or we can manually go to the initialize call and refund based on the params. just go to etherscan.io/tx/complete-bridge-tx-hash which will have all the data or take it from the etherscan.io/tx/initiate-bridge-tx-hash emitted event.
Or eth_getLogs initiateBridge tx hash

1. We have a permissioned refund, that has been discussed.
2. It's really not that complicated.
3. Saving gas is mandatory for any protocol operating on ethereum. If we have the opportunity to substantially lower it, I think it's a good take.

[apenzk]

Sorry, so we have an API that stores events, we would store the initaite bridge event and that data would serve to call refund. We can also have it in local memory while the functions are called. Or we could store it in local storage. Or we could retrieve it from etherscan's API by taking the function call arguments. Or we can manually go to the initialize call and refund based on the params. just go to etherscan.io/tx/complete-bridge-tx-hash which will have all the data or take it from the etherscan.io/tx/initiate-bridge-tx-hash emitted event.

1. We have a permissioned refund, that has been discussed.
2. It's really not that complicated.
3. Saving gas is mandatory for any protocol operating on ethereum. If we have the opportunity to substantially lower it, I think it's a good take.

@0xPrimata could you clarify which of this options is the one where the relevant information is extracted from on-chain data. Want to make sure i understand how our service gets the necessary information from onchain data and after it crashed, thereby loosing all data locally.

[Primata]

@apenzk We can reliably take the transaction input arguments from etherscan.io and their API.

[Primata]

Implementation available at https://github.com/movementlabsxyz/movement/tree/remove-bridge-struct

Gas Report

| src/NativeBridgeCounterpartyMOVE.sol:NativeBridgeCounterpartyMOVE contract |                 |       |        |       |         |
|----------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                            | Deployment Size |       |        |       |         |
| 659978                                                                     | 2834            |       |        |       |         |
| Function Name                                                              | min             | avg   | median | max   | # calls |
| abortBridgeTransfer                                                        | 2711            | 6347  | 3035   | 28757 | 9       |
| bridgeTransfers                                                            | 554             | 554   | 554    | 554   | 5       |
| completeBridgeTransfer                                                     | 944             | 10410 | 944    | 76518 | 8       |
| initialize                                                                 | 92870           | 92870 | 92870  | 92870 | 6       |
| lockBridgeTransfer                                                         | 5243            | 6003  | 5243   | 10568 | 21      |


| src/NativeBridgeInitiatorMOVE.sol:NativeBridgeInitiatorMOVE contract |                 |       |        |       |         |
|----------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                      | Deployment Size |       |        |       |         |
| 784340                                                               | 3409            |       |        |       |         |
| Function Name                                                        | min             | avg   | median | max   | # calls |
| bridgeTransfers                                                      | 513             | 513   | 513    | 513   | 2       |
| completeBridgeTransfer                                               | 3118            | 6369  | 3118   | 28975 | 8       |
| initialize                                                           | 92904           | 92904 | 92904  | 92904 | 6       |
| initiateBridgeTransfer                                               | 69223           | 69223 | 69223  | 69223 | 3       |
| refundBridgeTransfer                                                 | 2702            | 10240 | 3032   | 63817 | 9       |
| setCounterpartyAddress                                               | 24702           | 24702 | 24702  | 24702 | 6       |
| withdrawMOVE                                                         | 37552           | 37552 | 37552  | 37552 | 1       |

[franck44]

Sorry, so we have an API that stores events, we would store the initaite bridge event and that data would serve to call refund.

That does not work. You have to store on Ethereum mainnet, not in your own DB.

We can also have it in local memory while the functions are called.
Or we could store it in local storage.

Not sure you understand what global storage is.
"local memory" or "local storage" as you call it (I don't know what that is) is volatile. It exists only during the execution of a transaction.

Or we could retrieve it from etherscan's API by taking the function call arguments.

Etherscan is not a trusted API. That's not an option.

Or we can manually go to the initialize call and refund based on the params. just go to etherscan.io/tx/complete-bridge-tx-hash which will have all the data or take it from the etherscan.io/tx/initiate-bridge-tx-hash emitted event.

That's a very complicated answer unsecured process. It is not practical.

We have a permissioned refund, that has been discussed.
It's really not that complicated.

It is too complicated and not secure because either involves untrusted components.

Saving gas is mandatory for any protocol operating on ethereum. If we have the opportunity to substantially lower it, I think it's a good take.

The safe approach is to ensure security first and optimise second.
As I mentioned before, it costs 20000 gas per slot you write in global storage, and 130K gas is not that expensive.

[Primata]

@franck44
We are literally building a layer 2, how can you say "you have to store on Ethereum". There are plenty of protocols relying on off-chain solutions. Just as an example, off-chain signatures for permissioned transaction of tokens is widely used and requires an off-chain service, nothing is stored on chain, only once it's executed.

I meant local computer memory and storage, the app being ran on browser or localStorage.set()

That is very debatable and I think that off-loading user's cost is very promising.

I dispute that. It's very simple, the Relayer gets the data from events. Oh ya there is one more method, eth_getLogs. That should be enough, you just have to parse the logs and input it directly as arguments. We can even store on Movement but not on Ethereum and we will have a completely onchain storage of all required data for all bridge attempts.

This is just as secure as the current method, even more secure because we are currently not hash checking between the initiator and counterparty. Ethereum txs are exhorbitant and can get even more prohibitive expensive. In my life experience, if anything bad happens, a lot of people end up getting locked out because of how expensive it gets, especially retail, I have seen that happen and their losses become massive because they cannot exit. Anything we can do to improve that, and still have it secure, I think is good.

btw, we are saving:
initiate from 200k gas to 69k
lock 130k to 10k
complete counterparty from 85k to 75k
complete initiatiator from 30k to 28k

[franck44]

@0xPrimata

For Mainnet we don't need that, so we'll hold off on this proposal.

[apenzk]

@0xPrimata tbh a saving of only approximately 60% of cost at this point does not justify a short term fix of something where there seems to be a fundamental debate. Just wanting to express my support for holding off on a quick fix.

In particular the following MUST be our standard

The safe approach is to ensure security first and optimise second.

[Primata]

@0xPrimata tbh a saving of only approximately 60% of cost at this point does not justify a short term fix of something where there seems to be a fundamental debate. Just wanting to express my support for holding off on a quick fix.

In particular the following MUST be our standard

The safe approach is to ensure security first and optimise second.

This is totally fine, I understand it would be unsafe to implement this, but I definitely think it's at least as safe as the current implementation. This is mostly just a V2 proposed change. Totally fine with holding off on the proposal @franck44 .appreciate the response @apenzk .