feature/PRO-819/broadcast-confirmation-for-evm-transactions #4044

Janislav · 2023-09-22T11:33:49Z

Pull Request

Closes: PRO-xxx

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

linear · 2023-09-22T11:33:52Z

PRO-819 Broadcast confirmation for EVM transactions

When we request a transaction broadcast for EVM, we are trusting the validators to sign and send a transaction without tampering.

We also set a very high cap on the gas limit for most transactions (15M gas).

There is a possible attack vector whereby a validator could create a 'forwarding' contract that consumes any excess gas up to the limit, and forwards the calldata to the chainflip contract. The net effect would be that the KeyManager emits the correct event and we witness the transaction as succeeded, and register the consumed gas to be refunded to the validator.

A validator could use this to drain funds from the vault (assuming we do refund the validator, which is something we intend to do!).

As a solution, we should re-instate the signed transaction checks for these cases: in order to become whitelisted for a fee refund, a validator must prove that they signed the specific transaction without altering the transaction details.

(Specific transaction details for the evm case are

See https://github.com/chainflip-io/chainflip-backend/blob/chore/mainnet-ccm-gaslimit-test/state-chain/chains/src/evm.rs#L422)

Initial outline here:

#3968

codecov · 2023-09-22T11:43:21Z

Codecov Report

Merging #4044 (151b53e) into main (4b2d251) will decrease coverage by 1%.
Report is 33 commits behind head on main.
The diff coverage is 8%.

❗ Current head 151b53e differs from pull request most recent head 1e53f6b. Consider uploading reports for the commit 1e53f6b to get more accurate results

@@           Coverage Diff           @@
##            main   #4044     +/-   ##
=======================================
- Coverage     72%     72%     -1%     
=======================================
  Files        366     369      +3     
  Lines      57530   59497   +1967     
  Branches   57530   59497   +1967     
=======================================
+ Hits       41585   42683   +1098     
- Misses     13858   14690    +832     
- Partials    2087    2124     +37

Files	Coverage Δ
state-chain/chains/src/lib.rs	`40% <ø> (ø)`
state-chain/chains/src/mocks.rs	`59% <ø> (ø)`
state-chain/chains/src/none.rs	`0% <ø> (ø)`
state-chain/runtime/src/lib.rs	`39% <ø> (-<1%)`	⬇️
engine/src/eth/rpc.rs	`0% <0%> (ø)`
state-chain/chains/src/btc.rs	`85% <0%> (+1%)`	⬆️
state-chain/chains/src/dot.rs	`41% <0%> (-<1%)`	⬇️
state-chain/pallets/cf-broadcast/src/mock.rs	`90% <60%> (-3%)`	⬇️
state-chain/chains/src/eth.rs	`65% <0%> (-7%)`	⬇️
engine/src/state_chain_observer/sc_observer/mod.rs	`36% <0%> (-1%)`	⬇️
... and 3 more

... and 91 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dandanlen

Sorry, took me a while to to get round to looking at this. I think there are a few issues. I doubt it works right now.

dandanlen · 2023-10-04T21:15:11Z

state-chain/pallets/cf-broadcast/src/lib.rs

+			// If the signature is valid, we can remove the transaction from storage
+			Transaction::<T, I>::remove(broadcast_attempt_id.broadcast_id);
+			// And mark the broadcast as validated
+			BroadcastValidated::<T, I>::insert(broadcast_attempt_id.broadcast_id, ());


We need to save the hash here so we can compare it at the broadcast success stage.

dandanlen · 2023-10-04T21:44:40Z

state-chain/chains/src/evm.rs

+		signature: Vec<u8>,
+		tx_hash: H256,
+	) -> Result<(), TransactionVerificationError> {
+		let recovered = Self::recover_transaction(&signature)?;


It's not possible to recover a transaction from a signature.

dandanlen · 2023-10-04T21:45:41Z

state-chain/chains/src/evm.rs

+	) -> Result<(), TransactionVerificationError> {
+		let recovered = Self::recover_transaction(&signature)?;
+		let hash = Self::recover_hash(recovered.clone());
+		ensure!(hash == tx_hash, TransactionVerificationError::InvalidTransactionHash);


We need to do this after broadcast, not before.

dandanlen · 2023-10-04T21:47:35Z

engine/src/state_chain_observer/sc_observer/mod.rs

+                                                                    broadcast_attempt_id,
+                                                                    transaction: transaction_payload.clone(),
+                                                                    transaction_signature: eth_rpc.sign_transaction(transaction_payload).await?.to_vec(),
+                                                                    transaction_hash: tx_hash,


We don't need to submit this here.

dandanlen · 2023-10-04T21:51:25Z

engine/src/state_chain_observer/sc_observer/mod.rs

+                                                                    transaction: transaction_payload.clone(),
+                                                                    transaction_signature: eth_rpc.sign_transaction(transaction_payload).await?.to_vec(),


Here want the raw signed payload. The steps to get this are something like:

let transaction_request = Eip1559TransactionRequest::from(transaction_payload); let signature = eth_rpc.sign_transaction(transaction_request).await?; let typed_transaction = TypedTransaction::from(transaction_request); let raw_signed_payload = typed_transaction.rlp_signed(&signature);

dandanlen · 2023-10-04T21:52:08Z

state-chain/pallets/cf-broadcast/src/lib.rs

@@ -192,6 +202,13 @@ pub mod pallet {
 		/// The save mode block margin
 		type SafeModeBlockMargin: Get<BlockNumberFor<Self>>;

+		/// The thing thats validates the transaction
+		type TransactionValidator: TransactionValidator<


The trait could be simpler: TransactionValidator<C: Chain>.

dandanlen · 2023-10-04T21:53:05Z

state-chain/pallets/cf-broadcast/src/lib.rs

-			TransactionFeeDeficit::<T, I>::mutate(signer_id, |fee_deficit| {
-				*fee_deficit = fee_deficit.saturating_add(to_refund);
-			});
+			if let Some(_) = BroadcastValidated::<T, I>::take(broadcast_id) {


This is where we need to check the tx hash.

dandanlen · 2023-10-04T21:55:28Z

engine/src/state_chain_observer/sc_observer/mod.rs

+                                                            state_chain_runtime::RuntimeCall::EthereumBroadcaster(
+                                                                pallet_cf_broadcast::Call::confirm_broadcast_attempt {
+                                                                    broadcast_attempt_id,
+                                                                    transaction: transaction_payload.clone(),


We are just resubmitting the same transaction that was emitted in the event. It needs to be the transaction that was actually signed. (actually the signed rlp payload, see the other comment).

dandanlen · 2023-10-04T21:58:51Z

Please hold up before fixing this. There might be a simpler way. (See Linear)

Janislav · 2023-10-06T06:47:36Z

Okay. I start a new PR then.

Janislav · 2023-10-06T15:01:03Z

Closing this one now. Started a new one: #4078

Janislav added 4 commits September 20, 2023 14:50

feat: added state-chain specific buisness logic

d17679f

feat: ✨ added signaure verfication

4273e8a

refactor: ♻️ refactored error handling

a57cad1

fix: 🐛 fixed test compilation

a61b25d

Janislav added 4 commits September 26, 2023 11:14

feat: ✨ added sign and send transaction to CFE

151b53e

feat: ✨ added transaction hash validation

9ff4a58

refactor: ♻️ changed BroadcastValidated storage item

a4a1213

feat: added benchmark, added tests

1e53f6b

Janislav changed the title ~~Feature/pro 819/broadcast confirmation for evm transactions~~ feature/PRO-819/broadcast-confirmation-for-evm-transactions Oct 4, 2023

dandanlen reviewed Oct 4, 2023

View reviewed changes

Janislav closed this Oct 6, 2023

ahasna deleted the feature/PRO-819/broadcast-confirmation-for-evm-transactions branch May 23, 2024 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/PRO-819/broadcast-confirmation-for-evm-transactions #4044

feature/PRO-819/broadcast-confirmation-for-evm-transactions #4044

Janislav commented Sep 22, 2023

linear bot commented Sep 22, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

dandanlen left a comment

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen Oct 4, 2023

dandanlen commented Oct 4, 2023

Janislav commented Oct 6, 2023

Janislav commented Oct 6, 2023

		transaction: transaction_payload.clone(),
		transaction_signature: eth_rpc.sign_transaction(transaction_payload).await?.to_vec(),

feature/PRO-819/broadcast-confirmation-for-evm-transactions #4044

feature/PRO-819/broadcast-confirmation-for-evm-transactions #4044

Conversation

Janislav commented Sep 22, 2023

Pull Request

Checklist

Summary

linear bot commented Sep 22, 2023

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

dandanlen left a 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

dandanlen commented Oct 4, 2023

Janislav commented Oct 6, 2023

Janislav commented Oct 6, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading