[Bridge] [Solidity] Remove BridgeTransfer Struct

[0xPrimata]

Is your feature request related to a problem? Please describe. Our bridge is currently extremely expensive as shown in our gas analysis: https://github.com/movementlabsxyz/movement/issues/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. https://github.com/movementlabsxyz/movement/issues/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.

[0xPrimata]

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?

[0xPrimata]

@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.

[0xPrimata]

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.

[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.

[0xPrimata]

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

[0xPrimata]

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       |