Merge branch 'gdemay/XC-108-guard-against-potential-double-minting' i…

…nto 'master'

fix(ckerc20): Prevent with a guard potential double minting of ckETH and ckERC20 [override-didc-check]

(XC-108): Fix a potential source of double minting for ckETH and ckERC20 deposits, since the state modification ensuring that an event is not minting twice happens in the callback, after having contacted the ledger to mint the corresponding tokens. This approach is a bit brittle, because in the unlikely case where this callback would panic, state would be reverted and therefore the same event could be minted twice. 

This MR prevents double minting by adding a scope guard that will remove the event from the queue of events to mint and will in particular be executed if the callback were to panic. A drawback of this approach is that in the worst case, a not minted deposit due to a ledger error and a panicking callback would result in a lost deposit and would require manual intervention (note that the Ethereum transaction making the deposit is of course still available on Ethereum). However, this is still preferable to the previous (unlikely) situation where a minted deposit and a panicking callback would lead to double minting.

Remark on `[override-didc-check]`: Change on the Candid interface only impacts `get_events`, which is a debug endpoint. 

See merge request dfinity-lab/public/ic!18797

rs/ethereum/cketh/minter/BUILD.bazel

@@ -62,6 +62,7 @@ rust_library(
         "@crate_index//:num-bigint",
         "@crate_index//:num-traits",
         "@crate_index//:rlp",
+        "@crate_index//:scopeguard",
         "@crate_index//:serde",
         "@crate_index//:serde_bytes",
         "@crate_index//:serde_json",

rs/ethereum/cketh/minter/Cargo.toml

@@ -38,6 +38,7 @@ num-bigint = "0.4.3"
 num-traits = { workspace = true }
 phantom_newtype = { path = "../../../phantom_newtype" }
 rlp = "0.5.2"
+scopeguard = "1.1.0"
 serde = { workspace = true }
 serde_bytes = { workspace = true }
 serde_json = { workspace = true }

rs/ethereum/cketh/minter/cketh_minter.did

@@ -424,6 +424,9 @@ type Event = record {
             mint_block_index : nat;
             ckerc20_token_symbol: text;
         };
+        QuarantinedDeposit : record {
+            event_source : EventSource;
+        };
     };
 };
 

rs/ethereum/cketh/minter/src/dashboard.rs

@@ -15,7 +15,7 @@ use ic_cketh_minter::numeric::{
 use ic_cketh_minter::state::transactions::{
     ReimbursementIndex, TransactionCallData, WithdrawalRequest,
 };
-use ic_cketh_minter::state::{EthBalance, MintedEvent, State};
+use ic_cketh_minter::state::{EthBalance, InvalidEventReason, MintedEvent, State};
 use ic_cketh_minter::tx::Eip1559TransactionRequest;
 use ic_ethereum_types::Address;
 use std::cmp::Reverse;
@@ -132,7 +132,7 @@ pub struct DashboardTemplate {
     pub ledger_id: Principal,
     pub minted_events: Vec<MintedEvent>,
     pub pending_deposits: Vec<DashboardPendingDeposit>,
-    pub rejected_deposits: BTreeMap<EventSource, String>,
+    pub invalid_events: BTreeMap<EventSource, InvalidEventReason>,
     pub withdrawal_requests: Vec<DashboardWithdrawalRequest>,
     pub pending_transactions: Vec<DashboardPendingTransaction>,
     pub finalized_transactions: Vec<DashboardFinalizedTransaction>,
@@ -301,7 +301,7 @@ impl DashboardTemplate {
             last_observed_block: state.last_observed_block_number,
             minted_events,
             pending_deposits,
-            rejected_deposits: state.invalid_events.clone(),
+            invalid_events: state.invalid_events.clone(),
             withdrawal_requests,
             pending_transactions,
             finalized_transactions,

rs/ethereum/cketh/minter/src/dashboard/tests.rs

@@ -334,15 +334,15 @@ fn should_display_rejected_deposits() {
             &vec![
                 "0x05c6ec45699c9a6a4b1a4ea2058b0cee852ea2f19b18fb8313c04bf8156efde4",
                 "11",
-                "failed to decode principal",
+                "Invalid deposit: failed to decode principal",
             ],
         )
         .has_rejected_deposits(
             2,
             &vec![
                 "0x09a5ee10c942f99b79cabcfb9647fc06e79489c6a8e96d39faed4f3ac6bc83d3",
                 "0",
-                "failed to decode principal",
+                "Invalid deposit: failed to decode principal",
             ],
         );
 }

rs/ethereum/cketh/minter/src/deposit.rs

@@ -11,6 +11,7 @@ use hex_literal::hex;
 use ic_canister_log::log;
 use ic_ethereum_types::Address;
 use num_traits::ToPrimitive;
+use scopeguard::ScopeGuard;
 use std::cmp::{min, Ordering};
 use std::time::Duration;
 
@@ -33,6 +34,18 @@ async fn mint() {
     let mut error_count = 0;
 
     for event in events {
+        // Ensure that even if we were to panic in the callback, after having contacted the ledger to mint the tokens,
+        // this event will not be processed again.
+        let prevent_double_minting_guard = scopeguard::guard(event.clone(), |event| {
+            mutate_state(|s| {
+                process_event(
+                    s,
+                    EventType::QuarantinedDeposit {
+                        event_source: event.source(),
+                    },
+                )
+            });
+        });
         let (token_symbol, ledger_canister_id) = match &event {
             ReceivedEvent::Eth(_) => ("ckETH".to_string(), eth_ledger_canister_id),
             ReceivedEvent::Erc20(event) => {
@@ -68,6 +81,8 @@ async fn mint() {
             Ok(Err(err)) => {
                 log!(INFO, "Failed to mint {token_symbol}: {event:?} {err}");
                 error_count += 1;
+                // minting failed, defuse guard
+                ScopeGuard::into_inner(prevent_double_minting_guard);
                 continue;
             }
             Err(err) => {
@@ -76,6 +91,8 @@ async fn mint() {
                     "Failed to send a message to the ledger ({ledger_canister_id}): {err:?}"
                 );
                 error_count += 1;
+                // minting failed, defuse guard
+                ScopeGuard::into_inner(prevent_double_minting_guard);
                 continue;
             }
         };
@@ -103,6 +120,8 @@ async fn mint() {
             event.value(),
             event.principal()
         );
+        // minting succeeded, defuse guard
+        ScopeGuard::into_inner(prevent_double_minting_guard);
     }
 
     if error_count > 0 {

rs/ethereum/cketh/minter/src/endpoints.rs

@@ -380,5 +380,8 @@ pub mod events {
             ckerc20_token_symbol: String,
             erc20_contract_address: String,
         },
+        QuarantinedDeposit {
+            event_source: EventSource,
+        },
     }
 }

rs/ethereum/cketh/minter/src/main.rs

@@ -705,6 +705,9 @@ fn get_events(arg: GetEventsArg) -> GetEventsResult {
                     to,
                     to_subaccount: to_subaccount.map(|s| s.0),
                 },
+                EventType::QuarantinedDeposit { event_source } => EP::QuarantinedDeposit {
+                    event_source: map_event_source(event_source),
+                },
             },
         }
     }

rs/ethereum/cketh/minter/src/state.rs

@@ -19,6 +19,7 @@ use ic_crypto_ecdsa_secp256k1::PublicKey;
 use ic_ethereum_types::Address;
 use std::cell::RefCell;
 use std::collections::{btree_map, BTreeMap, BTreeSet, HashSet};
+use std::fmt::{Display, Formatter};
 use strum_macros::EnumIter;
 use transactions::EthTransactions;
 
@@ -63,7 +64,7 @@ pub struct State {
     pub last_observed_block_number: Option<BlockNumber>,
     pub events_to_mint: BTreeMap<EventSource, ReceivedEvent>,
     pub minted_events: BTreeMap<EventSource, MintedEvent>,
-    pub invalid_events: BTreeMap<EventSource, String>,
+    pub invalid_events: BTreeMap<EventSource, InvalidEventReason>,
     pub eth_transactions: EthTransactions,
     pub skipped_blocks: BTreeSet<BlockNumber>,
 
@@ -110,6 +111,33 @@ pub enum InvalidStateError {
     InvalidLastErc20ScrapedBlockNumber(String),
 }
 
+#[derive(Debug, Eq, PartialEq, Clone)]
+pub enum InvalidEventReason {
+    /// Deposit is invalid and was never minted.
+    /// This is most likely due to a user error (e.g., user's IC principal cannot be decoded)
+    /// or there is a critical issue in the logs returned from the JSON-RPC providers.
+    InvalidDeposit(String),
+
+    /// Deposit is valid but it's unknown whether it was minted or not,
+    /// most likely because there was an unexpected panic in the callback.
+    /// The deposit is quarantined to avoid any double minting and
+    /// will not be further processed without manual intervention.
+    QuarantinedDeposit,
+}
+
+impl Display for InvalidEventReason {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self {
+            InvalidEventReason::InvalidDeposit(reason) => {
+                write!(f, "Invalid deposit: {}", reason)
+            }
+            InvalidEventReason::QuarantinedDeposit => {
+                write!(f, "Quarantined deposit")
+            }
+        }
+    }
+}
+
 impl State {
     pub fn validate_config(&self) -> Result<(), InvalidStateError> {
         if self.ecdsa_key_name.trim().is_empty() {
@@ -233,6 +261,20 @@ impl State {
             .map(|(symbol, _, _)| symbol)
     }
 
+    /// Quarantine the deposit event to prevent double minting.
+    /// WARNING!: It's crucial that this method does not panic,
+    /// since it's called inside the clean-up callback, when an unexpected panic did occur before.
+    fn record_quarantined_deposit(&mut self, source: EventSource) -> bool {
+        self.events_to_mint.remove(&source);
+        match self.invalid_events.entry(source) {
+            btree_map::Entry::Occupied(_) => false,
+            btree_map::Entry::Vacant(entry) => {
+                entry.insert(InvalidEventReason::QuarantinedDeposit);
+                true
+            }
+        }
+    }
+
     fn record_invalid_deposit(&mut self, source: EventSource, error: String) -> bool {
         assert!(
             !self.events_to_mint.contains_key(&source),
@@ -246,7 +288,7 @@ impl State {
         match self.invalid_events.entry(source) {
             btree_map::Entry::Occupied(_) => false,
             btree_map::Entry::Vacant(entry) => {
-                entry.insert(error);
+                entry.insert(InvalidEventReason::InvalidDeposit(error));
                 true
             }
         }

rs/ethereum/cketh/minter/src/state/audit.rs

@@ -141,6 +141,9 @@ pub fn apply_state_transition(state: &mut State, payload: &EventType) {
                 cketh_reimbursement_request.clone(),
             )
         }
+        EventType::QuarantinedDeposit { event_source } => {
+            state.record_quarantined_deposit(*event_source);
+        }
     }
 }
 

rs/ethereum/cketh/minter/src/state/audit/tests.rs

@@ -418,6 +418,9 @@ impl GetEventsFile {
                     ckerc20_token_symbol,
                     erc20_contract_address: erc20_contract_address.parse().unwrap(),
                 },
+                EventPayload::QuarantinedDeposit { event_source } => ET::QuarantinedDeposit {
+                    event_source: map_event_source(event_source),
+                },
             },
         }
     }

rs/ethereum/cketh/minter/src/state/event.rs

@@ -140,6 +140,15 @@ pub enum EventType {
     /// The minter could not burn the given amount of ckERC20 tokens.
     #[n(20)]
     FailedErc20WithdrawalRequest(#[n(0)] ReimbursementRequest),
+    /// The minter unexpectedly panic while processing a deposit.
+    /// The deposit is quarantined to prevent any double minting and
+    /// will not be processed without further manual intervention.
+    #[n(21)]
+    QuarantinedDeposit {
+        /// The unique identifier of the deposit on the Ethereum network.
+        #[n(0)]
+        event_source: EventSource,
+    },
 }
 
 impl ReceivedEvent {

rs/ethereum/cketh/minter/src/state/tests.rs

@@ -69,7 +69,7 @@ mod mint_transaction {
     use crate::lifecycle::EthereumNetwork;
     use crate::numeric::{LedgerMintIndex, LogIndex};
     use crate::state::tests::{initial_state, received_erc20_event, received_eth_event};
-    use crate::state::MintedEvent;
+    use crate::state::{InvalidEventReason, MintedEvent};
 
     #[test]
     fn should_record_mint_task_from_event() {
@@ -198,10 +198,29 @@ mod mint_transaction {
         assert_ne!(error, other_error);
 
         assert!(state.record_invalid_deposit(event.source(), error.to_string()));
-        assert_eq!(state.invalid_events[&event.source()], error.to_string());
+        assert_eq!(
+            state.invalid_events[&event.source()],
+            InvalidEventReason::InvalidDeposit(error.to_string())
+        );
 
         assert!(!state.record_invalid_deposit(event.source(), other_error.to_string()));
-        assert_eq!(state.invalid_events[&event.source()], error.to_string());
+        assert_eq!(
+            state.invalid_events[&event.source()],
+            InvalidEventReason::InvalidDeposit(error.to_string())
+        );
+    }
+
+    #[test]
+    fn should_quarantine_deposit() {
+        let mut state = initial_state();
+        let event = received_eth_event();
+        state.record_event_to_mint(&event.clone().into());
+        assert_eq!(state.events_to_mint.len(), 1);
+
+        state.record_quarantined_deposit(event.source());
+
+        assert!(state.events_to_mint.is_empty());
+        assert!(state.invalid_events.contains_key(&event.source()));
     }
 
     #[test]
@@ -784,7 +803,7 @@ fn state_equivalence() {
     use crate::state::transactions::{
         EthTransactions, EthWithdrawalRequest, Reimbursed, ReimbursementRequest,
     };
-    use crate::state::MintedEvent;
+    use crate::state::{InvalidEventReason, MintedEvent};
     use crate::tx::{
         Eip1559Signature, Eip1559TransactionRequest, SignedTransactionRequest, TransactionRequest,
     };
@@ -1016,7 +1035,7 @@ fn state_equivalence() {
             }
         },
         invalid_events: btreemap! {
-            source("0x05c6ec45699c9a6a4b1a4ea2058b0cee852ea2f19b18fb8313c04bf8156efde4", 11) => "failed to decode principal from bytes 0x00333c125dc9f41abaf2b8b85d49fdc7ff75b2a4000000000000000000000000".to_string(),
+            source("0x05c6ec45699c9a6a4b1a4ea2058b0cee852ea2f19b18fb8313c04bf8156efde4", 11) => InvalidEventReason::InvalidDeposit("failed to decode principal from bytes 0x00333c125dc9f41abaf2b8b85d49fdc7ff75b2a4000000000000000000000000".to_string()),
         },
         eth_transactions: eth_transactions.clone(),
         pending_withdrawal_principals: Default::default(),

rs/ethereum/cketh/minter/templates/dashboard.html

@@ -277,7 +277,7 @@ <h3 id="minted-events">Minted events</h3>
             </table>
             {% endif %}
 
-            {% if !rejected_deposits.is_empty() %}
+            {% if !invalid_events.is_empty() %}
             <h3 id="rejected-deposits">Rejected deposits</h3>
             <table>
                 <thead>
@@ -288,11 +288,11 @@ <h3 id="rejected-deposits">Rejected deposits</h3>
                     </tr>
                 </thead>
                 <tbody>
-                    {% for (source, error) in rejected_deposits %}
+                    {% for (source, reason) in invalid_events %}
                     <tr>
                         <td>{% call etherscan_tx_link(source.transaction_hash.to_string()) %}</td>
                         <td class="numeric">{{ source.log_index }}</td>
-                        <td>{{ error }}</td>
+                        <td>{{ reason }}</td>
                     </tr>
                     {% endfor %}
                 </tbody>