feat: verify transaction metadata (#4078)(PRO-819) #4078
Conversation
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 Initial outline here: |
Codecov Report
@@ Coverage Diff @@
## main #4078 +/- ##
======================================
- Coverage 72% 72% -0%
======================================
Files 378 378
Lines 61153 61335 +182
Branches 61153 61335 +182
======================================
+ Hits 43876 43998 +122
- Misses 15007 15062 +55
- Partials 2270 2275 +5
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
state-chain/chains/src/btc.rs
Outdated
| // There is no need for replay protection on Bitcoin since it is a UTXO chain. | ||
| type ReplayProtectionParams = (); | ||
| type ReplayProtection = (); | ||
| } | ||
|
|
||
| pub struct BitcoinTransactionMetaDataHandler; | ||
|
|
||
| impl TransactionMetaDataHandler<Bitcoin> for BitcoinTransactionMetaDataHandler { |
There was a problem hiding this comment.
This could just be a default blanket impl on ().
impl<C: Chain> TransactionMetaDataHandler<C> for () {
// ...
}There was a problem hiding this comment.
I did this, it removes a lot of code.
| log::warn!( | ||
| "Transaction metadata verification failed for broadcast {}. Validator can not get refunded.", | ||
| broadcast_id | ||
| ); |
There was a problem hiding this comment.
We could have an event for this too - ValidatorRefundRefused?
There was a problem hiding this comment.
I renamed these events to TransactionFeeDeficitRecorded/TransactionFeeDeficitRefused.
I think it makes it clearer that (a) it's not always a validator (btc and dot fees are paid by the vault) and (b) it's not refunded, just recorded for later possible refunds.
|
I've implemented the required changes or explained why I think we should not change it. |
- Consistent naming MetaData -> Metadata - Remove redundant trait defs - Factor out some repetition - Define trait in terms of Self - Define generic EvmTransactionMetadata - Remove storage at cleanup - Rename events to reflect that we don't refund and that it's not always a validator.
|
I made some refactors and added a test. LGTM, thanks! |
|
Actually before merging, @kylezs can you sanity check the engine changes please? |
dandanlen
changed the title
Pull Request
Closes: PRO-819
Checklist
Please conduct a thorough self-review before opening the PR.
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.