Contract: Add invalid recipient list (#147)

contract/src/contract.rs

@@ -8,8 +8,8 @@ use crate::{
     msg::{
         AvailableTicketsResponse, BridgeStateResponse, CoreumTokensResponse, ExecuteMsg,
         FeesCollectedResponse, InstantiateMsg, PendingOperationsResponse, PendingRefund,
-        PendingRefundsResponse, ProcessedTxsResponse, QueryMsg, TransactionEvidence,
-        TransactionEvidencesResponse, XRPLTokensResponse,
+        PendingRefundsResponse, ProcessedTxsResponse, ProhibitedXRPLRecipientsResponse, QueryMsg,
+        TransactionEvidence, TransactionEvidencesResponse, XRPLTokensResponse,
     },
     operation::{
         check_operation_exists, create_pending_operation,
@@ -21,8 +21,8 @@ use crate::{
     state::{
         BridgeState, Config, ContractActions, CoreumToken, TokenState, UserType, XRPLToken,
         AVAILABLE_TICKETS, CONFIG, COREUM_TOKENS, FEES_COLLECTED, PENDING_OPERATIONS,
-        PENDING_REFUNDS, PENDING_ROTATE_KEYS, PENDING_TICKET_UPDATE, PROCESSED_TXS, TX_EVIDENCES,
-        USED_TICKETS_COUNTER, XRPL_TOKENS,
+        PENDING_REFUNDS, PENDING_ROTATE_KEYS, PENDING_TICKET_UPDATE, PROCESSED_TXS,
+        PROHIBITED_XRPL_RECIPIENTS, TX_EVIDENCES, USED_TICKETS_COUNTER, XRPL_TOKENS,
     },
     tickets::{
         allocate_ticket, handle_ticket_allocation_confirmation, register_used_ticket, return_ticket,
@@ -39,7 +39,7 @@ use coreum_wasm_sdk::{
 };
 use cosmwasm_std::{
     coin, coins, entry_point, to_json_binary, Addr, BankMsg, Binary, Coin, CosmosMsg, Deps,
-    DepsMut, Env, MessageInfo, Order, Response, StdResult, Storage, Uint128,
+    DepsMut, Empty, Env, MessageInfo, Order, Response, StdResult, Storage, Uint128,
 };
 use cw2::set_contract_version;
 use cw_ownable::{get_ownership, initialize_owner, is_owner, Action};
@@ -83,6 +83,14 @@ pub const MIN_DENOM_LENGTH: usize = 3;
 pub const MAX_DENOM_LENGTH: usize = 128;
 pub const DENOM_SPECIAL_CHARACTERS: [char; 5] = ['/', ':', '.', '_', '-'];
 
+pub const INITIAL_PROHIBITED_XRPL_RECIPIENTS: [&str; 5] = [
+    "rrrrrrrrrrrrrrrrrrrrrhoLvTp", // ACCOUNT_ZERO: An address that is the XRP Ledger's base58 encoding of the value 0. In peer-to-peer communications, rippled uses this address as the issuer for XRP.
+    "rrrrrrrrrrrrrrrrrrrrBZbvji", // ACCOUNT_ONE: An address that is the XRP Ledger's base58 encoding of the value 1. In the ledger, RippleState entries use this address as a placeholder for the issuer of a trust line balance.
+    "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", // Genesis account: When rippled starts a new genesis ledger from scratch (for example, in stand-alone mode), this account holds all the XRP. This address is generated from the seed value masterpassphrase which is hard-coded.
+    "rrrrrrrrrrrrrrrrrNAMEtxvNvQ", // Ripple Name reservation black-hole: In the past, Ripple asked users to send XRP to this account to reserve Ripple Names.
+    "rrrrrrrrrrrrrrrrrrrn5RM1rHd", // NaN Address: Previous versions of ripple-lib generated this address when encoding the value NaN using the XRP Ledger's base58 string encoding format.
+];
+
 #[cfg_attr(not(feature = "library"), entry_point)]
 pub fn instantiate(
     deps: DepsMut<CoreumQueries>,
@@ -128,7 +136,7 @@ pub fn instantiate(
         evidence_threshold: msg.evidence_threshold,
         used_ticket_sequence_threshold: msg.used_ticket_sequence_threshold,
         trust_set_limit_amount: msg.trust_set_limit_amount,
-        bridge_xrpl_address: msg.bridge_xrpl_address,
+        bridge_xrpl_address: msg.bridge_xrpl_address.clone(),
         bridge_state: BridgeState::Active,
         xrpl_base_fee: msg.xrpl_base_fee,
     };
@@ -166,6 +174,12 @@ pub fn instantiate(
     let key = build_xrpl_token_key(XRP_ISSUER, XRP_CURRENCY);
     XRPL_TOKENS.save(deps.storage, key, &token)?;
 
+    // We store all the prohibited recipients in state, including the multisig address, which is also prohibited to send to
+    for address in INITIAL_PROHIBITED_XRPL_RECIPIENTS {
+        PROHIBITED_XRPL_RECIPIENTS.save(deps.storage, address.to_string(), &Empty {})?;
+    }
+    PROHIBITED_XRPL_RECIPIENTS.save(deps.storage, msg.bridge_xrpl_address, &Empty {})?;
+
     Ok(Response::new()
         .add_attribute("action", ContractActions::Instantiation.as_str())
         .add_attribute("contract_name", CONTRACT_NAME)
@@ -308,6 +322,13 @@ pub fn execute(
             new_relayers,
             new_evidence_threshold,
         ),
+        ExecuteMsg::UpdateProhibitedXRPLRecipients {
+            prohibited_xrpl_recipients,
+        } => update_prohibited_xrpl_recipients(
+            deps.into_empty(),
+            info.sender,
+            prohibited_xrpl_recipients,
+        ),
     }
 }
 
@@ -903,9 +924,8 @@ fn send_to_xrpl(
     // Check that the recipient is a valid XRPL address
     validate_xrpl_address(&recipient)?;
 
-    // We prohibit to send to the multisig address
-    let config = CONFIG.load(deps.storage)?;
-    if recipient.eq(&config.bridge_xrpl_address) {
+    // We don't allow sending to a prohibited recipient
+    if PROHIBITED_XRPL_RECIPIENTS.has(deps.storage, recipient.clone()) {
         return Err(ContractError::ProhibitedRecipient {});
     }
 
@@ -1327,6 +1347,39 @@ fn rotate_keys(
         .add_attribute("sender", sender))
 }
 
+fn update_prohibited_xrpl_recipients(
+    deps: DepsMut,
+    sender: Addr,
+    prohibited_xrpl_recipients: Vec<String>,
+) -> CoreumResult<ContractError> {
+    check_authorization(
+        deps.as_ref().storage,
+        &sender,
+        &ContractActions::UpdateProhibitedXRPLRecipients,
+    )?;
+
+    // We clear the previous prohibited recipients
+    PROHIBITED_XRPL_RECIPIENTS.clear(deps.storage);
+
+    // We add the current multisig address which is always prohibited
+    let config = CONFIG.load(deps.storage)?;
+    PROHIBITED_XRPL_RECIPIENTS.save(deps.storage, config.bridge_xrpl_address, &Empty {})?;
+
+    // Add all prohibited recipients provided
+    for prohibited_xrpl_recipient in prohibited_xrpl_recipients {
+        // Validate the address that we are adding, to not add useless things
+        validate_xrpl_address(&prohibited_xrpl_recipient)?;
+        PROHIBITED_XRPL_RECIPIENTS.save(deps.storage, prohibited_xrpl_recipient, &Empty {})?;
+    }
+
+    Ok(Response::new()
+        .add_attribute(
+            "action",
+            ContractActions::UpdateProhibitedXRPLRecipients.as_str(),
+        )
+        .add_attribute("sender", sender))
+}
+
 // ********** Queries **********
 #[cfg_attr(not(feature = "library"), entry_point)]
 pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> {
@@ -1372,6 +1425,9 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> {
             start_after_key,
             limit,
         } => to_json_binary(&query_processed_txs(deps, start_after_key, limit)),
+        QueryMsg::ProhibitedXRPLRecipients {} => {
+            to_json_binary(&query_prohibited_xrpl_recipients(deps))
+        }
     }
 }
 
@@ -1568,6 +1624,18 @@ fn query_processed_txs(
     }
 }
 
+fn query_prohibited_xrpl_recipients(deps: Deps) -> ProhibitedXRPLRecipientsResponse {
+    let prohibited_xrpl_recipients: Vec<String> = PROHIBITED_XRPL_RECIPIENTS
+        .range(deps.storage, None, None, Order::Ascending)
+        .filter_map(Result::ok)
+        .map(|(addr, _)| addr)
+        .collect();
+
+    ProhibitedXRPLRecipientsResponse {
+        prohibited_xrpl_recipients,
+    }
+}
+
 // ********** Helpers **********
 
 fn check_issue_fee(deps: &DepsMut<CoreumQueries>, info: &MessageInfo) -> Result<(), ContractError> {

contract/src/error.rs

@@ -173,7 +173,7 @@ pub enum ContractError {
     )]
     OperationVersionMismatch {},
 
-    #[error("ProhibitedRecipient: The recipient cannot be the bridge")]
+    #[error("ProhibitedRecipient: The recipient is a prohibited address")]
     ProhibitedRecipient {},
 
     #[error("DeliverAmountIsProhibited: Optional deliver_amount field is only used for XRPL originated tokens (except XRP) being bridged back")]

contract/src/msg.rs

@@ -140,6 +140,12 @@ pub enum ExecuteMsg {
         new_relayers: Vec<Relayer>,
         new_evidence_threshold: u32,
     },
+    // Update the prohibited recipients list
+    // Only the owner can do this
+    #[serde(rename = "update_prohibited_xrpl_recipients")]
+    UpdateProhibitedXRPLRecipients {
+        prohibited_xrpl_recipients: Vec<String>,
+    },
 }
 
 #[cw_ownable_query]
@@ -190,6 +196,9 @@ pub enum QueryMsg {
         start_after_key: Option<String>,
         limit: Option<u32>,
     },
+    #[returns(ProhibitedXRPLRecipientsResponse)]
+    #[serde(rename = "prohibited_xrpl_recipients")]
+    ProhibitedXRPLRecipients {},
 }
 
 #[cw_serde]
@@ -255,3 +264,8 @@ pub struct ProcessedTxsResponse {
     pub last_key: Option<String>,
     pub processed_txs: Vec<String>,
 }
+
+#[cw_serde]
+pub struct ProhibitedXRPLRecipientsResponse {
+    pub prohibited_xrpl_recipients: Vec<String>,
+}

contract/src/signatures.rs

@@ -62,7 +62,7 @@ pub fn add_signature(
     Ok(())
 }
 
-fn validate_signature(signature: &String) -> Result<(), ContractError> {
+fn validate_signature(signature: &str) -> Result<(), ContractError> {
     // The purpose of this function is to avoid attacks
     // We set a max length of 200, a reasonable length, here to avoid spam attack by a malicious relayer that wants to send a very long signature for an operation
     // And to also not error out in case a relayer sends a signature that is a bit longer than the one we expect

contract/src/state.rs

@@ -24,6 +24,7 @@ pub enum TopKey {
     FeesCollected = b'c',
     FeeRemainders = b'd',
     PendingRotateKeys = b'e',
+    ProhibitedXRPLRecipients = b'f',
 }
 
 impl TopKey {
@@ -194,6 +195,9 @@ pub const FEES_COLLECTED: Map<Addr, Vec<Coin>> = Map::new(TopKey::FeesCollected.
 // Fees Remainders in case that we have some small amounts left after dividing fees between our relayers we will keep them here until next time we collect fees and can add them to the new amount
 // Key is Coin denom and value is Coin amount
 pub const FEE_REMAINDERS: Map<String, Uint128> = Map::new(TopKey::FeeRemainders.as_str());
+// XRPL addresses that have been marked as prohibited and can't be used for receiving funds
+pub const PROHIBITED_XRPL_RECIPIENTS: Map<String, Empty> =
+    Map::new(TopKey::ProhibitedXRPLRecipients.as_str());
 
 pub enum ContractActions {
     Instantiation,
@@ -208,6 +212,7 @@ pub enum ContractActions {
     UpdateXRPLToken,
     UpdateCoreumToken,
     UpdateXRPLBaseFee,
+    UpdateProhibitedXRPLRecipients,
     ClaimRefunds,
     HaltBridge,
     ResumeBridge,
@@ -234,6 +239,7 @@ impl UserType {
             ContractActions::UpdateXRPLToken => matches!(self, Self::Owner),
             ContractActions::UpdateCoreumToken => matches!(self, Self::Owner),
             ContractActions::UpdateXRPLBaseFee => matches!(self, Self::Owner),
+            ContractActions::UpdateProhibitedXRPLRecipients => matches!(self, Self::Owner),
             ContractActions::ClaimRefunds => true,
             ContractActions::HaltBridge => matches!(self, Self::Owner | Self::Relayer),
             ContractActions::ResumeBridge => matches!(self, Self::Owner),
@@ -258,6 +264,7 @@ impl ContractActions {
             Self::UpdateXRPLToken => "update_xrpl_token",
             Self::UpdateCoreumToken => "update_coreum_token",
             Self::UpdateXRPLBaseFee => "update_xrpl_base_fee",
+            Self::UpdateProhibitedXRPLRecipients => "update_invalid_xrpl_recipients",
             Self::HaltBridge => "halt_bridge",
             Self::ResumeBridge => "resume_bridge",
             Self::RotateKeys => "rotate_keys",

contract/src/tests.rs

@@ -20,10 +20,10 @@ mod tests {
     use std::collections::HashMap;
 
     use crate::address::validate_xrpl_address;
-    use crate::contract::MAX_RELAYERS;
+    use crate::contract::{INITIAL_PROHIBITED_XRPL_RECIPIENTS, MAX_RELAYERS};
     use crate::msg::{
-        BridgeStateResponse, ProcessedTxsResponse, TransactionEvidence,
-        TransactionEvidencesResponse,
+        BridgeStateResponse, ProcessedTxsResponse, ProhibitedXRPLRecipientsResponse,
+        TransactionEvidence, TransactionEvidencesResponse,
     };
     use crate::state::BridgeState;
     use crate::{
@@ -650,7 +650,7 @@ mod tests {
                 evidence_threshold: 3,
                 used_ticket_sequence_threshold: 5,
                 trust_set_limit_amount: Uint128::new(TRUST_SET_LIMIT_AMOUNT),
-                bridge_xrpl_address,
+                bridge_xrpl_address: bridge_xrpl_address.clone(),
                 bridge_state: BridgeState::Active,
                 xrpl_base_fee: 10,
             }
@@ -795,6 +795,73 @@ mod tests {
             .unwrap();
 
         assert!(!query_transaction_evidence.relayer_addresses.is_empty());
+
+        // Let's query the prohibited recipients
+        let query_prohibited_recipients = wasm
+            .query::<QueryMsg, ProhibitedXRPLRecipientsResponse>(
+                &contract_addr,
+                &QueryMsg::ProhibitedXRPLRecipients {},
+            )
+            .unwrap();
+
+        assert_eq!(
+            query_prohibited_recipients.prohibited_xrpl_recipients.len(),
+            INITIAL_PROHIBITED_XRPL_RECIPIENTS.len() + 1
+        );
+        assert!(query_prohibited_recipients
+            .prohibited_xrpl_recipients
+            .contains(&bridge_xrpl_address));
+
+        // Let's try to update this by adding a new one and query again
+        let new_prohibited_recipient = generate_xrpl_address();
+        let mut prohibited_recipients = query_prohibited_recipients
+            .prohibited_xrpl_recipients
+            .clone();
+        prohibited_recipients.push(new_prohibited_recipient.clone());
+        wasm.execute::<ExecuteMsg>(
+            &contract_addr,
+            &ExecuteMsg::UpdateProhibitedXRPLRecipients {
+                prohibited_xrpl_recipients: prohibited_recipients,
+            },
+            &vec![],
+            &signer,
+        )
+        .unwrap();
+
+        let query_prohibited_recipients = wasm
+            .query::<QueryMsg, ProhibitedXRPLRecipientsResponse>(
+                &contract_addr,
+                &QueryMsg::ProhibitedXRPLRecipients {},
+            )
+            .unwrap();
+
+        assert_eq!(
+            query_prohibited_recipients.prohibited_xrpl_recipients.len(),
+            INITIAL_PROHIBITED_XRPL_RECIPIENTS.len() + 2
+        );
+        assert!(query_prohibited_recipients
+            .prohibited_xrpl_recipients
+            .contains(&bridge_xrpl_address));
+
+        assert!(query_prohibited_recipients
+            .prohibited_xrpl_recipients
+            .contains(&new_prohibited_recipient));
+
+        // If we try to update this from an account that is not the owner it will fail
+        let update_error = wasm
+            .execute::<ExecuteMsg>(
+                &contract_addr,
+                &ExecuteMsg::UpdateProhibitedXRPLRecipients {
+                    prohibited_xrpl_recipients: vec![],
+                },
+                &vec![],
+                &relayer_accounts[0],
+            )
+            .unwrap_err();
+
+        assert!(update_error
+            .to_string()
+            .contains(ContractError::UnauthorizedSender {}.to_string().as_str()));
     }
 
     #[test]
@@ -3155,6 +3222,23 @@ mod tests {
             .to_string()
             .contains(ContractError::ProhibitedRecipient {}.to_string().as_str()));
 
+        // If we try to send tokens from Coreum to XRPL using a prohibited recipient address, it should fail.
+        let bridge_error = wasm
+            .execute::<ExecuteMsg>(
+                &contract_addr,
+                &ExecuteMsg::SendToXRPL {
+                    recipient: INITIAL_PROHIBITED_XRPL_RECIPIENTS[0].to_string(),
+                    deliver_amount: None,
+                },
+                &coins(1, denom_xrp.clone()),
+                sender,
+            )
+            .unwrap_err();
+
+        assert!(bridge_error
+            .to_string()
+            .contains(ContractError::ProhibitedRecipient {}.to_string().as_str()));
+
         // Sending a CoreumToXRPLTransfer evidence with account sequence should fail.
         let invalid_evidence = wasm
             .execute::<ExecuteMsg>(
@@ -10114,7 +10198,7 @@ mod tests {
 
     #[test]
     fn validate_xrpl_addresses() {
-        let valid_addresses = vec![
+        let mut valid_addresses = vec![
             "rU6K7V3Po4snVhBBaU29sesqs2qTQJWDw1".to_string(),
             "rLUEXYuLiQptky37CqLcm9USQpPiz5rkpD".to_string(),
             "rBTwLga3i2gz3doX6Gva3MgEV8ZCD8jjah".to_string(),
@@ -10128,6 +10212,11 @@ mod tests {
             generate_xrpl_address(),
         ];
 
+        // Add the current prohibited recipients and check that they are valid generated xrpl addresses
+        for prohibited_recipient in INITIAL_PROHIBITED_XRPL_RECIPIENTS {
+            valid_addresses.push(prohibited_recipient.to_string());
+        }
+
         for address in valid_addresses.iter() {
             validate_xrpl_address(address).unwrap();
         }