Fast Transfers #147

kiseln · 2024-11-28T12:21:37Z

Description

Fast transfer is initiated by sending tokens to the bridge contract with a proper message for ft_on_transfer.

Recipient on Near

Tokens are transferred directly to the recipient
During finalization tokens and fees are released back to the relayer

Recipient on other chain

Tokens are burned/locked and a new transfer is created for a 3rd-party chain.
During finalization tokens are released to the relayer
Fees are paid only after finalizing on 3rd-party chain

Implementation

msg has been changed for ft_on_transfer to resolve between init_transfer and fast_fin_transfer.
Fast transfers are stored in a lookup with a key being a hash of its struct. Relayer must provide correct transfer_id, token_id, recipient, amount, fee, and msg.
Storage deposit is only performed if recipient is on Near in order to be able to transfer tokens to the recipient. Storage deposit is done using relayer balance instead of attached NEAR.
Any transfers requiring native NEAR are done using wNEAR. If recipient is on Near, the tokens are unwrapped before sending.
Only relayer that performed fast transfer can then finalize the transfer

Concerns

For other chain transfers sender is changed to be the relayer. I don't think this parameter is important but keeping in mind that it is different from normal transfers.
Storage deposits need to be double checked and tested

near/omni-bridge/src/lib.rs

near/omni-types/src/lib.rs

olga24912 · 2024-12-10T12:05:13Z

near/omni-types/src/lib.rs

+    pub recipient: OmniAddress,
+    pub fee: Fee,
+    pub msg: String,
+    pub storage_deposit_amount: Option<u128>,


And could you add comment, when the storage_deposit should be provided and for what.

near/omni-bridge/src/lib.rs

olga24912 · 2024-12-10T12:31:03Z

near/omni-bridge/src/lib.rs

+
+        let OmniAddress::Near(recipient) = fast_transfer.recipient.clone() else {
+            env::panic_str("ERR_INVALID_STATE")
+        };


In the ft_on_transfer function, we burned the tokens. Since an error occurred here, we need to roll back the token burn.

This would never happen because this method is only called if recipient is on Near

In any case, the error could have occurred during the previous check. In that case, the tokens would already be burned, and we wouldn’t roll back the operation. The balance wouldn’t match.

Actually if it's a bridged token (the only one we burn) bridge balance would be zero. Meaning if relayer sends an incorrect transaction the tokens will be burned and the refund to the relayer will fail. There will be no loss of funds from the bridge perspective, right? @karim-en

Added tests and a fix here 88a9781
Not sure if more elegant solution is possible

near/omni-bridge/src/lib.rs

olga24912 · 2024-12-10T14:14:43Z

near/omni-bridge/src/lib.rs

+            .add_transfer_message(transfer_message, relayer_id.clone())
+            .saturating_add(required_balance);
+
+        self.update_storage_balance(relayer_id, required_balance, env::attached_deposit());


env::attached_deposit() -- this value isn’t always zero, is it?

Changed to 0

near/omni-bridge/src/lib.rs

olga24912 · 2024-12-10T14:24:37Z

near/omni-bridge/src/lib.rs

+            token_id: token_id.clone(),
+            recipient: fast_fin_transfer_msg.recipient.clone(),
+            amount: U128(amount.0 + fast_fin_transfer_msg.fee.fee.0),
+            fee: fast_fin_transfer_msg.fee,


Who will ultimately receive the native fee? The Fast Transfer Relayer or the General Relayer? Are we at risk of spending the same funds twice?

If recipient is on Near, the fast relayer will get the fee after finalizing the original transfer.
If recipient is on another chain, it's a bit tricky. The fee is claimed only after transfer is finalized on the 3rd chain. And right now it can be another relayer. Perhaps we need to split the fee between the two relayers in this scenario @karim-en ?

Perhaps we can allow sign_transfer only for the fast relayer in this case

Edit: but then fast relayer will be able to finalize the original transfer while keeping the 3rd-chain message unsigned. Need to think more

Actually, in case recipient is on another chain we need a solution for both normal and fast transfers. So perhaps it's not quite in scope of this PR. As of now, the relayer that finalizes the transfer on the destination chain will receive the fees

near/omni-bridge/src/lib.rs

olga24912 · 2024-12-10T14:50:45Z

I might also consider adding an option to opt out of fast-transfer.

karim-en · 2024-12-10T23:57:00Z

near/omni-bridge/src/lib.rs

+                    )
+                    .then(
+                        Self::ext(env::current_account_id())
+                            .with_static_gas(VERIFY_PROOF_CALLBACK_GAS)


The const VERIFY_PROOF_CALLBACK_GAS isn't correct

Fixed 1f38b04

karim-en · 2024-12-11T00:08:59Z

@kiseln Please add integration tests to this new feature

kiseln · 2024-12-12T07:38:24Z

@kiseln Please add integration tests to this new feature

118a333

karim-en · 2024-12-13T04:31:15Z

near/omni-bridge/src/lib.rs

@@ -875,6 +1022,14 @@ impl Contract {
    pub fn get_current_destination_nonce(&self, chain_kind: ChainKind) -> Nonce {
        self.destination_nonces.get(&chain_kind).unwrap_or_default()
    }
+
+    #[private]
+    pub fn ft_resolve_transfer(&mut self, amount: U128) -> U128 {


The naming could be confusing (in the explorer) because the nep141 implementation also has a ft_resolve_transfer

Renamed 1f38b04

karim-en · 2024-12-13T04:33:07Z

near/omni-bridge/src/lib.rs

+    pub fn ft_resolve_transfer(&mut self, amount: U128) -> U128 {
+        match env::promise_result(0) {
+            PromiseResult::Successful(_) => U128(0),
+            PromiseResult::Failed => amount,


This refund is dangerous due to the undetermined behavior of the ft_trnasfer_call, so let's always return zero.

Okay, just to confirm, if ft_transfer_call reverts it's sender who will lose money and not the relayer, right? So senders will be responsible for the target of ft_transfer_call

Changed to 0 1f38b04

Yes.
In the follow-up PR, we can try to solve the refund issue, which was mentioned multiple times in the audits.
What we should take into account is:

The ft_transfer_call can fail only if the storage wasn't deposited or due to malicious contract, otherwise it returns value.

The mint call ft_transfer_call if there is a message, so it has the same behaviour.

The ft_transfer can fail only if the storage wasn't deposited.

Since we already verify the storage deposit, then we can do the refund only if the ft_transfer_call returned a refund value.

karim-en · 2024-12-16T23:46:00Z

near/omni-bridge/src/lib.rs

+                    )
+                    .then(
+                        Self::ext(env::current_account_id())
+                            .with_static_gas(FAST_TRANSFER_CALLBACK_GAS)


Suggested change

.with_static_gas(FAST_TRANSFER_CALLBACK_GAS)

.with_static_gas(FAST_TRANSFER_CALLBACK_GAS + FT_TRANSFER_CALL_GAS)

This could be not enough to do the ft_transfer_call in the callback

Can you please also add tests for this flow (failed and success)

Fixed 88a9781

karim-en · 2024-12-16T23:47:07Z

near/omni-bridge/src/lib.rs

@@ -55,6 +56,8 @@ const DEPLOY_TOKEN_GAS: Gas = Gas::from_tgas(50);
 const BURN_TOKEN_GAS: Gas = Gas::from_tgas(10);
 const MINT_TOKEN_GAS: Gas = Gas::from_tgas(5);
 const SET_METADATA_GAS: Gas = Gas::from_tgas(10);
+const RESOLVE_TRANSFER_GAS: Gas = Gas::from_tgas(3);
+const FAST_TRANSFER_CALLBACK_GAS: Gas = Gas::from_tgas(40);


This can be reduced if the FT_TRANSFER_CALL_GAS has been added

olga24912 · 2024-12-18T13:47:58Z

near/omni-bridge/src/lib.rs

+        .then(
+            Self::ext(env::current_account_id())
+                .with_static_gas(RESOLVE_TRANSFER_GAS)
+                .resolve_transfer(fast_transfer.amount),


Suggested change

.resolve_transfer(fast_transfer.amount),

.resolve_transfer(fast_transfer.amount.0 - fast_transfer.fee.fee.0),

If we ever use the amount in resolve_transfer it will be for the purpose of rolling back ft_transfer_call so full amount is more appropriate

olga24912 · 2024-12-18T14:08:47Z

near/omni-bridge/src/lib.rs

+            token: OmniAddress::Near(fast_transfer.token_id.clone()),
+            amount: fast_transfer.amount.clone(),
+            recipient: fast_transfer.recipient.clone(),
+            fee: fast_transfer.fee.clone(),


What about creating an invalid TransferMessage with a very very large fee? Finalizing it on the destination chain, and then claiming our excessively large fee. In that case, a lot of Ether, for example, would be minted for the relayer without reason.

Fantastic find - this is a severe problem that we need to address. Also, replacing sender to the relayer's Near address we accidentally change the native fee from another token to NEAR.

First, we need to allow claiming fee only after the original message is finalized and fees validated.
Second, we either need to preserve original sender when creating message or look it up in some other way when claiming the fee.

I added restriction on claiming fee in 333be17

kiseln added 2 commits November 28, 2024 16:13

Implement simple fast transfer

a8dff43

run fmt

938da6c

kiseln requested review from karim-en and olga24912 November 29, 2024 08:13

karim-en requested changes Dec 1, 2024

View reviewed changes

near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved

Fast transfers implementation

527e148

kiseln force-pushed the fast-transfers branch from a6fc3bc to 527e148 Compare December 5, 2024 12:47

kiseln marked this pull request as ready for review December 5, 2024 13:46

kiseln requested a review from karim-en December 5, 2024 13:46

kiseln changed the title ~~Implement simple fast transfer~~ Fast Transfers Dec 5, 2024

Storage deposit and fee fixes

c1cd7f3

kiseln force-pushed the fast-transfers branch from b092ed6 to c1cd7f3 Compare December 5, 2024 14:27

Merge branch 'main' into fast-transfers

7143b16

karim-en requested changes Dec 5, 2024

View reviewed changes

near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved

near/omni-bridge/src/lib.rs Outdated Show resolved Hide resolved

near/omni-bridge/src/lib.rs Show resolved Hide resolved

Minor code review fixes

7aa8115

kiseln requested a review from karim-en December 6, 2024 10:28

Fix fast transfer to other chain message

a86a62e

kiseln force-pushed the fast-transfers branch from fa617c4 to a86a62e Compare December 9, 2024 16:10

olga24912 requested changes Dec 10, 2024

View reviewed changes

karim-en reviewed Dec 10, 2024

View reviewed changes

add integration tests

118a333

kiseln requested a review from karim-en December 12, 2024 07:39

kiseln added 3 commits December 12, 2024 20:44

Merge branch 'main' into fast-transfers

a553dc0

Fix token refund

f046b2d

cargo fmt

ba82ea7

karim-en requested changes Dec 13, 2024

View reviewed changes

Code review fixes

1f38b04

Fail during fast transfer if transfer already finalised

a3d9a72

kiseln requested review from olga24912 and karim-en December 13, 2024 13:33

karim-en requested changes Dec 16, 2024

View reviewed changes

karim-en reviewed Dec 16, 2024

View reviewed changes

olga24912 requested changes Dec 18, 2024

View reviewed changes

Do not burn tokens during failed fast transfer

88a9781

kiseln force-pushed the fast-transfers branch from 082a011 to 88a9781 Compare December 19, 2024 17:33

kiseln added 4 commits January 29, 2025 13:57

Merge branch 'main' into fast-transfers

04dd5d8

Mark fast transfer as completed

cb763fc

Implement restriction on claiming fee before fast transfer is finalised

333be17

Merge branch 'main' into fast-transfers

4f6b386

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast Transfers #147

Fast Transfers #147

kiseln commented Nov 28, 2024 •

edited

Loading

olga24912 Dec 10, 2024

olga24912 Dec 10, 2024

kiseln Dec 10, 2024

olga24912 Dec 18, 2024

kiseln Dec 19, 2024

kiseln Dec 19, 2024

olga24912 Dec 10, 2024

kiseln Dec 12, 2024

olga24912 Dec 10, 2024

kiseln Dec 10, 2024

kiseln Dec 10, 2024 •

edited

Loading

kiseln Dec 13, 2024

olga24912 commented Dec 10, 2024

karim-en Dec 10, 2024

karim-en Dec 13, 2024

kiseln Dec 13, 2024

karim-en commented Dec 11, 2024 •

edited

Loading

kiseln commented Dec 12, 2024

karim-en Dec 13, 2024

kiseln Dec 13, 2024

karim-en Dec 13, 2024

kiseln Dec 13, 2024

kiseln Dec 13, 2024

karim-en Dec 16, 2024 •

edited

Loading

karim-en Dec 16, 2024

karim-en Dec 17, 2024

kiseln Dec 19, 2024

karim-en Dec 16, 2024

olga24912 Dec 18, 2024

kiseln Dec 19, 2024

olga24912 Dec 18, 2024

kiseln Dec 19, 2024

kiseln Jan 29, 2025

	.with_static_gas(FAST_TRANSFER_CALLBACK_GAS)
	.with_static_gas(FAST_TRANSFER_CALLBACK_GAS + FT_TRANSFER_CALL_GAS)

	.resolve_transfer(fast_transfer.amount),
	.resolve_transfer(fast_transfer.amount.0 - fast_transfer.fee.fee.0),

Fast Transfers #147

Are you sure you want to change the base?

Fast Transfers #147

Conversation

kiseln commented Nov 28, 2024 • edited Loading

Description

Recipient on Near

Recipient on other chain

Implementation

Concerns

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiseln Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olga24912 commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karim-en commented Dec 11, 2024 • edited Loading

kiseln commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karim-en Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiseln commented Nov 28, 2024 •

edited

Loading

kiseln Dec 10, 2024 •

edited

Loading

karim-en commented Dec 11, 2024 •

edited

Loading

karim-en Dec 16, 2024 •

edited

Loading